Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

H activation fails at abcnews.go.com #2397

Closed
judell opened this issue Jul 22, 2015 · 6 comments · Fixed by #2425
Closed

H activation fails at abcnews.go.com #2397

judell opened this issue Jul 22, 2015 · 6 comments · Fixed by #2425

Comments

@judell
Copy link
Contributor

judell commented Jul 22, 2015

URL: http://abcnews.go.com/Politics/lindsey-graham-destroyed-cellphone-spite-donald-trump/story?id=32621487

Symptom: Hypothesis does not activate

Console error in hypothesis.min.js:

Uncaught TypeError: Cannot read property 'Util' of undefined8.../annotation-sync @ hypothesis.min.js?bc659cac:2s @ hypothesis.min.js?bc659cac:1e @ hypothesis.min.js?bc659cac:1(anonymous function) @ hypothesis.min.js?bc659cac:1

@tilgovi
Copy link
Contributor

tilgovi commented Jul 23, 2015

require('annotator') is undefined. Not sure why.

@nickstenning nickstenning added Bug and removed P3 labels Jul 23, 2015
@robertknight
Copy link
Member

Minimal repro case:

<html>
<meta charset="UTF-8">
<body>
<script>
self = {}; // global = {} produces the same result

// Hypothesis bookmarklet
(function(){window.hypothesisConfig=function(){return{showHighlights:true};};var d=document,s=d.createElement('script');s.setAttribute('src','https://hypothes.is/app/embed.js');d.body.appendChild(s)})();
</script>
</body>
</html>

Happens in any context where self or global properties exists on the global context but are not actually the global objects themselves (so their properties are not part of the global context). In this page this happens in merlin.js.

This happens due to assumptions in browserify-shim about global, self and window:

// require('annotator') function from hypothesis.min.js:
(function(global) { // global === window.self
  ...
  (function browserifyShim(module, exports, require, define, browserify_shim__define__module__export__) {
    (function() {
      ...
      this.Annotator = Annotator; // this === window.self
      ...
    }).call(this);

    // neither Annotator nor window.Annotator defined here
    browserify_shim__define__module__export__(typeof Annotator != "undefined" ? Annotator : window.Annotator)
  }).call(global, undefined, undefined, undefined, undefined, function defineExport(ex) {
    // sets exports of 'annotator' module,
    // called as browserify_shim__define__module__export__(), with ex being undefined
    module.exports = ex
  })
}).call(this, typeof global !== "undefined" ? global : typeof self !== "undefined" ? self : typeof window !== "undefined" ? window : {})

@tilgovi
Copy link
Contributor

tilgovi commented Jul 30, 2015

It's not straightforwardly browserify-shim. It's because browserify-shim uses global, assuming that browserify will take care of defining it correctly. The relevant issue is browserify/insert-module-globals#48

tilgovi added a commit that referenced this issue Jul 31, 2015
The frame-rpc module is a proper CommonJS module and it's footprint
is really tiny. Its protocol is simpler. It doesn't handle connection
and buffering, but with our onConnect callback we don't need buffering.
The connection handshake that jschannel did was so trivial it's been
re-implemented here (each side tries to connect to the other once).

We have to handle timeouts ourselves, but that's all hidden in the
bridge code. In exchange, we get a simpler API, we get rid of the
call/notify distinction in favor of just passing a callback or not
and we avoid the excess overhead of the recursion guard in the
serialization code that was giving us false positive issues with
the document title.

This is one step toward removing all the browserify-shim requiring
libraries from the injected bundle, which will eventually fix #2397.
tilgovi added a commit that referenced this issue Jul 31, 2015
The frame-rpc module is a proper CommonJS module and it's footprint
is really tiny. Its protocol is simpler. It doesn't handle connection
and buffering, but with our onConnect callback we don't need buffering.
The connection handshake that jschannel did was so trivial it's been
re-implemented here (each side tries to connect to the other once).

We have to handle timeouts ourselves, but that's all hidden in the
bridge code. In exchange, we get a simpler API, we get rid of the
call/notify distinction in favor of just passing a callback or not
and we avoid the excess overhead of the recursion guard in the
serialization code that was giving us false positive issues with
the document title.

This is one step toward removing all the browserify-shim requiring
libraries from the injected bundle, which will eventually fix #2397.
tilgovi added a commit that referenced this issue Jul 31, 2015
The frame-rpc module is a proper CommonJS module and it's footprint
is really tiny. Its protocol is simpler. It doesn't handle connection
and buffering, but with our onConnect callback we don't need buffering.
The connection handshake that jschannel did was so trivial it's been
re-implemented here (each side tries to connect to the other once).

We have to handle timeouts ourselves, but that's all hidden in the
bridge code. In exchange, we get a simpler API, we get rid of the
call/notify distinction in favor of just passing a callback or not
and we avoid the excess overhead of the recursion guard in the
serialization code that was giving us false positive issues with
the document title.

This is one step toward removing all the browserify-shim requiring
libraries from the injected bundle, which will eventually fix #2397.
tilgovi added a commit that referenced this issue Jul 31, 2015
The frame-rpc module is a proper CommonJS module and it's footprint
is really tiny. Its protocol is simpler. It doesn't handle connection
and buffering, but with our onConnect callback we don't need buffering.
The connection handshake that jschannel did was so trivial it's been
re-implemented here (each side tries to connect to the other once).

We have to handle timeouts ourselves, but that's all hidden in the
bridge code. In exchange, we get a simpler API, we get rid of the
call/notify distinction in favor of just passing a callback or not
and we avoid the excess overhead of the recursion guard in the
serialization code that was giving us false positive issues with
the document title.

This is one step toward removing all the browserify-shim requiring
libraries from the injected bundle, which will eventually fix #2397.
@tilgovi
Copy link
Contributor

tilgovi commented Aug 3, 2015

I'll double check but don't think this can be closed yet.

My commit message said "This is one step toward removing all the browserify-shim requiring
libraries from the injected bundle, which will eventually fix #2397."

Only one step.

@tilgovi tilgovi reopened this Aug 3, 2015
@tilgovi
Copy link
Contributor

tilgovi commented Aug 4, 2015

I don't know what fixed this but it is fixed. The repro that @robertknight posts still causes problems. I've opened thlorenz/browserify-shim#183 to discuss, but given that we don't have an actual site report here for a real-world case where global or self is a global variable I'm going to close this and we'll inherit whatever fix ends up in browserify, insert-module-globals, or browserify-shim eventually for the unlikely edge case where this does turn out to be an issue.

@tilgovi tilgovi closed this as completed Aug 4, 2015
@robertknight
Copy link
Member

Looks like the code which triggered the issue in the 3rd-party website was rewritten from merlin.js to merlin-core.js, removing the assignment to the global self var in the process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants