794461 - `onload` fails on xraywrappers

Status: RESOLVED INVALID

(Core :: XPConnect, defect)

Description

[Alexandre Poirot [:ochameau]]

Bug 787063 from SDK exposed that issue.
The following code doesn't work (nothing happens) when `window` is an xraywrappers:
  window.onload = function () { ... }
Whereas following code works fine:
  window.addEventListener("load", function () {...}, false);

I submitted a workaround in SDK code until we can fix that on xraywrappers.

As usual, here is a scratchpad test case:
var obsService = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
obsService.addObserver({
    observe: function (subject, topic, data) {
        obsService.removeObserver(this, "document-element-inserted");
        var win = subject.defaultView;
        /* // workaround
        var onload = null;
        Object.defineProperty(win, "onload", {
          get: function () {
            return onload;
          },
          set: function (f) {
            if (typeof f == "function") {
                win.addEventListener("load", f, false);
            }
            if (onload)
               win.removeEventListener("load", onload, false);
            onload = f;
          },
          enumerable: true,
          configurable: false
        });*/
        
        // Works fine
        win.addEventListener("load", function() {
          Components.utils.reportError("addEventListener");
        }, false);
        // Nothing happen
        win.onload = function() {
          Components.utils.reportError("onload");
        };
    }
}, "document-element-inserted", false);

content.location = "http://google.fr?q=foo";

Comment 1

[Boris Zbarsky [:bzbarsky]]

That's odd. The fix for bug 659350 should have made this work....

Comment 2

[Boris Zbarsky [:bzbarsky]]

So what I see here is that the SetOnload is called correctly.  Then we wrap the given object into the content compartment (ending up with a xpc::ChromeObjectWrapper).

I'm not sure what happens after that.  Bobby?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and this is not an expando....

Comment 4

[Bobby Holley (:bholley)]

So, the onload handler is actually getting set up just fine. But then it gets overwritten by the HTML5 parser, presumably because google.fr is setting its own onload handler.

We don't do any multiplexing for different origins in nsEventListenerManager::SetEventHandlerToJsval. And with the current code, |window.onload = foo| from chrome and content are indistinguishable from a security perspective, because we enter the compartment of the window at the top of that function and rewrap the listener. This lack of origin multiplexing isn't web-visible, because XOWs filter out cross-origin onfoo access anyhow per the same origin policy.

Anyway, I think the Gecko code is probably fine here and that the jetpack workaround is appropriate if they want to support this kind of API. But I'll let bz make the final call before WONTFIXing here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, yes.  If the page sets onload itself, that will of course override your onload set, just like if the page set .title itself it would override you setting .title.  Again, because this is not an expando.

Note that the "workaround" shown above that redefines the onload property breaks its semantics and would change the behavior of web pages.  I really hope Jetpack is not shipping that!