Bug 787063: Fix window.onload in content scripts. #589

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@ZER0 ZER0 commented on the diff Sep 26, 2012
packages/api-utils/lib/content/worker.js
@@ -166,6 +166,21 @@ const WorkerSandbox = EventEmitter.compose({
get unsafeWindow() window.wrappedJSObject
});
+ // Bug 787063: Xraywrapprers fails to support window.onload expando.
+ // Workaround it until platform is fixed.
+ let onload = null;
+ merge(content, {
+ get onload() { return onload; },
+ set onload(f) {
+ if (onload)
+ content.removeEventListener("load", onload, false);
+ // Accept only functions.
+ onload = typeof(f) === "function" ? f : null;
+ if (onload)
+ content.addEventListener("load", onload, false);
+ }
+ });
+
@ZER0
ZER0 Sep 26, 2012

What about simplify in this way, and remove the onload variable as well:

merge(content, {
  get onload() { return window.wrappedJSObject.onload },
  set onload(f) { window.wrappedJSObject.onload = f }
});

?
It could be also be done using this.unsafeWindow that is shorter, but it's better avoid to have a dependency like that, even if it's a temporary fix.

@ochameau
ochameau Sep 26, 2012 Mozilla member

We can't do that as it would mean exposing our method f to content and overloading method set by the page itself.
We we set window.onload = f; in content script, f is set in expando list of the xraywrapper, so that the page itself can't see this expando, nor that the content script erase page expandos.

@ZER0
ZER0 Sep 26, 2012

Not sure what you mean, could you explain a bit more? The only issue I can see is about context object, but that was just a quick example, can be easily fix. But it seems you're referring to something else. I'm not sure what you meant with "exposing our method f to content and overloading method set by the page itself"; but especially the last part about expandos is not really clear to me.

@ZER0
ZER0 Sep 26, 2012

Had a quick chat in IRC with @ochameau, where basically he explained to me the security issues given by the use of that approach.

@ZER0 ZER0 commented on the diff Sep 26, 2012
packages/addon-kit/tests/test-page-mod.js
+ }, false);
+ },
+ onAttach: function(w) {
+ w.on("message", function (success) {
+ test.assert(success, "received both DOMContentLoaded and load events");
+ pageMod.destroy();
+ tab.close();
+ test.done();
+ });
+ }
+ });
+
+ tabs.open({
+ url: "about:credits",
+ onReady: function onReady(t) {
+ tab = t;
@ZER0
ZER0 Sep 26, 2012

I believe you can remove the onReady callback and the tab variable as well; just use w.tab inside onAttach, see worker.tab.

@ZER0 ZER0 commented on the diff Sep 26, 2012
packages/api-utils/lib/content/worker.js
@@ -166,6 +166,21 @@ const WorkerSandbox = EventEmitter.compose({
get unsafeWindow() window.wrappedJSObject
});
+ // Bug 787063: Xraywrapprers fails to support window.onload expando.
+ // Workaround it until platform is fixed.
+ let onload = null;
+ merge(content, {
+ get onload() { return onload; },
+ set onload(f) {
+ if (onload)
+ content.removeEventListener("load", onload, false);
@ZER0
ZER0 Sep 26, 2012

Nit: the 3rd parameter is optional, can be omitted and will be false by default

@KWierso KWierso closed this Dec 13, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment