960545 - Navigation caused by window.open should use the incumbent script's browsing context as the source browsing context

Status: NEW

(Core :: DOM: Navigation, defect)

Description

[Bob Owen (:bobowen)]

As per the spec here:
http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#dom-open

"The navigation must be done with the responsible browsing context specified by the incumbent settings object as the source browsing context."

The description below assumes the patch for bug 785310 has landed.

Currently, in |nsWindowWatcher::OpenWindowInternal|, it first tries to find a window with a name of |aName| and if it can't it creates a new one.

In between this code and the code that calls |nsDocShell::LoadURI| most of the code only runs if it is a new window, but some parts run even for existing windows e.g.:
|rv = ReadyOpenedDocShellItem(newDocShellItem, aParent, windowIsNew, _retval);|

This function results in the opener getting set, which from my reading of the spec here:
http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#dom-opener
is a bug (in fact there is a very old one for it already, Bug 174349).
I think some of the other code might change the target window's state as well.

This is a problem because ideally we should be doing the sandboxing checks for this in |nsDocShell::InternalLoad| (called by |nsDocShell::LoadURI|), but if we did, and then navigation was blocked, an existing window would still have had its state changed.
So we have to do the sandboxing checks just after we find an existing window and pass null as the source browsing context to prevent double-checking.
(This could cause problems if/when we come to implement the seamless attribute.)

I think |nsWindowWatcher::OpenWindowInternal| should be changed, so that any code that changes the window state before navigation, should only run for new windows.
This could probably be broken out into a different function to make it more readable.
Then we could pass the correct source browsing context and let the |nsDocShell::InternalLoad| sandboxing checks run as normal.

Not sure if this bug should live in 'Navigation' or 'DOM: Core & HTML'.

Comment 1

[Bobby Holley (:bholley)]

Yeah, nsWindowWatcher::OpenWindowInternal is totally insane. Anything you can do to make it better is very welcome. But here there be dragons - don't get too sucked in. ;-)