787063 - should window.onload work in a content script?

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

window.onload does not work in a content script currently, however unsafeWindow.onload does. This is surprising to web developers ( see http://stackoverflow.com/questions/12194275/accessing-the-window-object-in-firefox-addon-content-script and https://builder.addons.mozilla.org/package/150362/latest/ )

IMO we should either make sure the onload handler is fired eventally, or at the very least document the limitation.

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Alex, would removing content proxies fix this?

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 589

For some reason, xraywrappers doesn't support `onload` expando on window object, whereas `onclick` (for ex) works fine.
Here is a workaround until the platform is hopefully fixed.

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/589/commits

Pointer to Github pull-request

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/589/commits#attch-to-bugzilla

Pointer to Github pull-request

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

Believed me; I have no idea why the add-on for attach the pull request decided to create TWO pull request when I was just reading the github page... Just ignore them.

Comment 6

[Alexandre Poirot [:ochameau]]

Ok so based on bug 794461 comments 5 and 6, matteo, you were right: onload isn't an expando and shouldn't behave like one.

The patch provided here isn't necessary the best option. I would not say it is as bad as bz said in comment 6. We aren't breaking the page. The only thing that may not behave exactly like standards is the content script.
But the worst thing here isn't necessary the initial bug report, but the security hole introduced by `onload` behavior (I'm now wondering if we have the same issue with onclick and others). I wouldn't expect to be able to expose any JS value from the content script to the page when I'm not using unsafeWindow. (Remember that at some point we were talking about getting rid of unsafeWindow) But here we break this rule. When we do: window.onload = function f() {}; we expose this function `f` to the page. The page can call it, access it and eventually do nasty things with it.

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

Okay so. Personally I kinda of agreed with bz. Also, I checked how Google Chrome faces the same issue. Basically it works as bz said, the content script window is not the same as the page window, (so if you add `window.foo = 1` in content script won't be exposed to the window's page) except for the `onclick`, `onload`, and so on. So, the event handler are shared, therefore overridden each other, if you have in your page `window.onclick = function() { foo() }` and in your content script you do `window.onclick()` then `foo` is called (in the page) and viceversa. So, it's definitely a kind of "communication channel" that I'm not sure it's intended, because on other hand I kind of agreed with you as well when you said you wouldn't expect to be able to expose any JS value from the content script to the page when you're not using `unsafeWindow`.

Could you makes some clear example about the "nasty things" you mentioned? What are in concrete the problem we could face if we exposes the function to the page?

Comment 8

[Dave Townsend [:mossop]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #7)
> Okay so. Personally I kinda of agreed with bz. Also, I checked how Google
> Chrome faces the same issue. Basically it works as bz said, the content
> script window is not the same as the page window, (so if you add `window.foo
> = 1` in content script won't be exposed to the window's page) except for the
> `onclick`, `onload`, and so on. So, the event handler are shared, therefore
> overridden each other, if you have in your page `window.onclick = function()
> { foo() }` and in your content script you do `window.onclick()` then `foo`
> is called (in the page) and viceversa. So, it's definitely a kind of
> "communication channel" that I'm not sure it's intended, because on other
> hand I kind of agreed with you as well when you said you wouldn't expect to
> be able to expose any JS value from the content script to the page when
> you're not using `unsafeWindow`.
> 
> Could you makes some clear example about the "nasty things" you mentioned?
> What are in concrete the problem we could face if we exposes the function to
> the page?

We shouldn't be exposing anything to the page through setting window.onload. At the very least the page can use it to detect the add-on is installed, it can also call the onload function potentially tricking the add-on into doing something unexpected. Chances are that will never be an issue but setting stuff on window should be safe as we can make it, that's why we have unsafeWindow for the other stuff.

I think we should fix up the patch so the comments make sense and then take it. Thoughts?

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

So what do you think? We should land this patch as is even if, as Boris said:

> 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!

?

As I said, I agreed that we shouldn't expose any JS value from the content script to the page when we're not using `unsafeWindow`, however I agreed with Boris as well that this breaks the semantics and will change the behavior of web pages.

By the way, I don't think detection is a problem: how the page could detect which add-on is installed from `onload`? It doesn't have a reference to the function to compare, could compare only the `toString` version of the function, and even if Google Chrome permit that we could just return `function () {[native code]}` instead (we probably need to use `bind` in any case).

If we decided instead to shipping the workaround, we should at least remove the `window.onload` from content page if present, the break the semantics and behaviors of web pages a bit "less" – it means that from add-on you can override the `window.onload` of the content page, but from content page you can't override the `window.onload` of the add-on. So at least, we have a case where the `window.onload` is executed twice, only when the page adds the `onload` after the add-on.
But still, as Boris said, it's not quite the behavior expected from a web developer.

Another option, as also Jeff mentioned in his comment, is don't fix it but document it. That's actually won't break anything, neither code, behavior or semantic: you simply can't use `window.onload` to set your listener for security reasons, but just `addEventListener`; if you want to override the content `window.onload` you have to access to the real window, therefore use `unsafeWindow` (that, as the same explicitly said, it's "unsafe").

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to Dave Townsend (:Mossop) from comment #8)
> I think we should fix up the patch so the comments make sense and then take
> it. Thoughts?

+1
I think we should rephrase bz comment:

> 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!

It won't change the behavior of web pages. It will only introduce some subtle differences in the way how content scripts interact with the webpage.
Ideally content script shouldn't have access to any JS values of the page, as for the opposite.
So window.onload from the content script shouldn't be able to access any function set by the page; and the opposite.
We can do so without changing the behavior of the webpage itself.
But it opens the more general question about node.setAttribute("onclick", "...") and node.onclick = function () {} which are both not easy to define.
Current behavior of node.setAttribute("onclick", "myFunction()") is misleading and might be tweaked as well.

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

After discussing with Dave, and how suggested by Jeff as well, we decided to keep the current behavior and document it (see Bug 810204); and close this bug as a wontfix.