Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix($$RAFProvider): prevent a JavaScript error in Firefox #16192

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix($$RAFProvider): prevent a JavaScript error in Firefox #16192

wants to merge 3 commits into from

Conversation

dmuellner
Copy link

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Bug fix: prevent a JavaScript error in Firefox.

What is the current behavior? (You can also link to an open issue here)

Firefox raises a Javascript Error "TypeError: 'requestAnimationFrame' called on an object that does not implement interface Window." with animated elements. This is because Window.requestAnimationFrame() is called without binding to a Window instance in the function which is returned from $$RAFProvider().

What is the new behavior (if this is a feature change)?

No error message.

Does this PR introduce a breaking change?

Not that I know of.

Please check if the PR fulfills these requirements

Other information:

I was not able to run local tests.

Firefox raises a Javascript Error "TypeError: 'requestAnimationFrame' called on an object that does not implement interface Window." with animated elements. This is because Window.requestAnimationFrame() is called without binding to a Window instance in the function which is returned from $$RAFProvider().
src/ng/raf.js Outdated
@@ -13,9 +13,9 @@ function $$RAFProvider() { //rAF
var rafSupported = !!requestAnimationFrame;
var raf = rafSupported
? function(fn) {
var id = requestAnimationFrame(fn);
var id = requestAnimationFrame.bind($window, fn);
Copy link
Contributor

@frederikprijck frederikprijck Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you mean to use call instead of bind here ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Thanks! I'll commit a fix in a minute.

src/ng/raf.js Outdated
return function() {
cancelAnimationFrame(id);
cancelAnimationFrame.bind($window, id);
Copy link
Contributor

@frederikprijck frederikprijck Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you mean to use call instead of bind here ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Thanks! I'll commit a fix in a minute.

@dmuellner
Copy link
Author

Hi Frederik (or other reviewers),

the failed test only complains that the commit message (in my forked repository) does not adhere to the required format. Can this can be resolved when the pull request is merged? Apart from this, the code changes seem to pass the tests. Let me know if you require something else from me.

@frederikprijck
Copy link
Contributor

frederikprijck commented Aug 23, 2017

Can this can be resolved when the pull request is merged?

Commit message can be changed using interactive rebase.

Regarding the effecting change this creates, @gkalpak or @Narretz will have to review it. I only wanted to mention the usage of bind looked incorrect.

function sayHello() {
  console.log("hi");
}

sayHello();
sayHello.bind(window); // does NOT execute sayHello(), bind returns a method which is still supposed to be executed by invoking the returned function.
sayHello.call(window); // does execute sayHello()

@Narretz
Copy link
Contributor

Narretz commented Aug 23, 2017

Under which circumstances does this happen? It's reported like it happens always but this seems unlikely.

We need a test or at least a reproduction that shows the error.

@dmuellner
Copy link
Author

I had difficulties isolating the problem as requested. In particular, I still do not understand the difference in Firefox and Chromium behavior. I suggest to let this pull request rest until I know better what is going on on my particular system. Thanks to the reviewers anyway—this is not as easy as I thought initially.

@dmuellner dmuellner closed this Aug 23, 2017
@dmuellner
Copy link
Author

I am reopening this pull request with a minimal example to reliably reproduce the error that this pull request fixes.

I constructed a minimal browser extension in the directory test/extension. (Note: this is for testing/demonstration only. This addition is not meant to be merged with the main repository.) The extension adds the text “Hello!” to the Google logo on https://www.google.ch.

Instructions:

  • Create a local copy of angular.js in test/extension. Eg., download from http://ajax.googleapis.com/ajax/libs/angularjs/1.6.4/angular.js.
  • Test on Chromium: Go to “More tools” → “Extensions” → “Load unpackaged extension...” and choose the directory test/extension.
  • Open https://www.google.ch. If the extension is properly loaded, the message “Hello!” appears on top of the Google logo.
  • Check the Javascript console: no error.
  • Test on Firefox: Enter “about:debugging” in the address bar, click “Load Temporary Add-on”, and again choose the directory test/extension.
  • Open https://www.google.ch. If the extension is properly loaded, the message “Hello!” appears on top of the Google logo.
  • Check the Javascript console: TypeError: 'requestAnimationFrame' called on an object that does not implement interface Window.

This error vanishes with the changes to src/ng/raf.js in the pull request.

@dmuellner dmuellner reopened this Aug 28, 2017
@dmuellner
Copy link
Author

I am still not sure what exactly the difference between Firefox and Chromium is. Here is my current guess: it seems as if in Firefox, there are two different functions window.requestAnimationFrame and the global requestAnimationFrame. In Chromium, both identifiers seem to reference the same function.

Try it yourself: edit angular.js locally and insert the following function definition:

function compareRAF($window) {
  var rafWindow = $window.requestAnimationFrame;
  var rafGlobal = requestAnimationFrame;
  console.log("equal:", rafWindow == rafGlobal);
}

Then, insert compareRAF($window) at the beginning of $$RAFProvider() as follows:

function $$RAFProvider() { //rAF
  this.$get = ['$window', '$timeout', function($window, $timeout) {
    compareRAF($window);
    var requestAnimationFrame = ...

Now test the extension in both browsers as described above. Firefox reports equal: false, while Chromium prints equal: true.

Does this give anyone insight into the TypeError in Firefox?

@gkalpak
Copy link
Member

gkalpak commented Aug 30, 2017

I tried to reproduce, but couldn't get the extension to work on Firefox 55. Can you?

@dmuellner
Copy link
Author

Yes, I can get it to work in Firefox 55.0.2 (64-bit). Did you place a copy of angular.js in test/extension? Here is how it looks like on my computer:
screenshot from 2017-08-30 14 35 43
screenshot from 2017-08-30 14 33 22

@gkalpak
Copy link
Member

gkalpak commented Sep 5, 2017

I am using Firefox 55.0.3 (64-bit) (on Windows 10) and I can't get the temp extension to load when I include the angular.js file in content_scripts[0].js array:

"//": "This works"
"content_scripts": [
  {
    "matches": ["..."],
    "js": [
      "test.js"
    ]
  }
]

"//": "This doesn't :("
"content_scripts": [
  {
    "matches": ["..."],
    "js": [
      "test.js",
      "angular.js"
    ]
  }
]

@gkalpak
Copy link
Member

gkalpak commented Sep 5, 2017

But I was able to reproduce the problem with requestAnimationFrame in Firefox extensions. All you need is:

// test.js
var raf1 = requestAnimationFrame;
var raf2 = window.requestAnimationFrame;

console.log(raf1 === raf2);   // false

raf1(() => console.log(1));   // Works
raf2(() => console.log(2));   // TypeError: 'requestAnimationFrame' called on an object that does not implement interface Window.

I also tried with Firefox Nightly and it still doesn't work, although there is no error logged to the console 😕

TBH, this sounds like a bug in Firefox. I wonder what else might be affected (besides requestAnimationFrame) - if any. @dmuellner, could you file a bug report for Firefox and post the link here?

@dmuellner
Copy link
Author

dmuellner commented Sep 6, 2017

Hi @gkalpak,

(1) the demo extension works for me in Firefox 55.0.3 (32-bit) on Windows 10. However, since you are able to reproduce the problem without angular.js, let's not worry about this.

(2) If you don't see the error logged on the (primary) console, try the “Debug” button in Firefox's “about:debugging” page. This opens a second console, on which you will likely see the error message.

(3) I don't think that this is a bug in Firefox but I suspect that this is intended behavior. Let me know if you feel I should still report this to the Firefox community. Here are my arguments:

The error is due to the question what the global object is in the JavaScript scope when requestAnimationFrame is called. In a web browser, in a script on an HTML page, the global object is the global Window. Your example code

var raf1 = requestAnimationFrame;
var raf2 = window.requestAnimationFrame;
console.log(raf1 === raf2);

reports true if the JavaScript code is part of a normal <script> section.

However, if the JavaScript code is executed as part of an extension, the global object is not necessarily the global Window. Reference: https://developer.mozilla.org/en-US/docs/Glossary/Global_object. Apparently, Firefox and Chrome differ here: it seems that the global object is still a Window instance in Chrome but not in Firefox. The developer page referenced above does not explicitly mention browser extensions but is very clear that the global object is not always a Window.

To me, the sanest solution is to bind the requestAnimationFrame reference to the Window instance it was originally taken from. We do have a Window instance when the method reference is generated:

var requestAnimationFrame = $window.requestAnimationFrame ||
                            $window.webkitRequestAnimationFrame;

so we should call requestAnimationFrame (ie., the variable of the same name) on $window itself and not on the global object (of unspecified type).

@gkalpak
Copy link
Member

gkalpak commented Sep 6, 2017

(1) the demo extension works for me in Firefox 55.0.3 (32-bit) on Windows 10. However, since you are able to reproduce the problem without angular.js, let's not worry about this.

¯\_(ツ)_/¯

(2) If you don't see the error logged on the (primary) console, try the “Debug” button in Firefox's “about:debugging” page. This opens a second console, on which you will likely see the error message.

Like I said, I do see the error in Firefox 55. I don't see it in Firefox Nightly.

(3) I don't think that this is a bug in Firefox but I suspect that this is intended behavior. Let me know if you feel I should still report this to the Firefox community. Here are my arguments:
...

I still think this is a bug in Firefox. When calling a function previously assign to a variable (when in strict mode as we are here), it should run with this === undefined and not this === <global object> (as happens in non-strict mode.

Also, the following will work in a normal (non-extension) environment in Firefox:

var raf1 = requestAnimationFrame;
var raf2 = window.requestAnimationFrame;

raf1(cb);
raf1.call(undefined, cb);
raf1.call(null, cb);
raf1.call(this, cb);

raf2(cb);
raf2.call(undefined, cb);
raf2.call(null, cb);
raf2.call(window, cb);

From the above, only the last one works from an extension. IMO, this is a clear indication that something is wrong. I am fine working aroung this in AngularJS (as you suggested) - it won't be the first time we work around browser-specific bugs - but I would still report it to Firefox and see what they think.

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

Successfully merging this pull request may close these issues.

5 participants