Bug 28716 – Event listeners installed on a window object returned from window.open() don't work

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dmitry Titov]

The following sequence:

var w = window.open(...);
w.addEventListener('load', ...);

does not invoke the specified listener when the newly opened page loads. It does in FF3.5. Attached files t.html and w.html can be used to repro - save them on local disk and open t.html.

However, this:
var w = window.open(...);
w.foo = "abc";
preserves the 'foo' property for the newly-loaded window to see.

Blowing away event listeners looks like a bug in WebKit, since the HTML spec specifically says to use the same Window object, which implies the state of it should be preserved, including event listeners - at least in the same-origin navigations:

HTML5 spec http://www.whatwg.org/specs/web-apps/current-work/#create-a-document-object:
"Creating a new Document object: When a Document is created as part of the above steps, a new set of views along with the associated Window object must be created and associated with the Document, with one exception: if the browsing context's only entry in its session history is the about:blank Document that was added when the browsing context was created, and navigation is occurring with replacement enabled, and that Document has the same origin as the new Document, then the Window object and associated views of that Document must be used instead, and the document attribute of the AbstractView objects of those views must be changed to point to the new Document instead."

[Dmitry Titov]

Created attachment 38563 [details]
repro file t.html

[Dmitry Titov]

Created attachment 38564 [details]
repro file w.html

[Alexey Proskuryakov]

For some context: when window.open() returns, the associated frame holds a pointer to temporary document object, which is replaced with a real one as provisional load is committed. Thus, w.document event listeners or properties would be all lost in both Firefox and Safari.

Like Firefox, we don't do the same for window object, but we clear event listeners to prevent cross-origin attacks (I think). This seems inconsistent to me - it would seem that properties shouldn't be preserved cross-origin either. And indeed, there doesn't seem to be a reason to remove listeners in same origin case, as far as I can tell.

#0	0x03bbff40 in WebCore::DOMWindow::removeAllEventListeners at DOMWindow.cpp:1364
#1	0x03ae1213 in WebCore::Document::removeAllEventListeners at Document.cpp:1389
#2	0x03c44ee8 in WebCore::FrameLoader::stopLoading at FrameLoader.cpp:585
#3	0x03c451c0 in WebCore::FrameLoader::closeURL at FrameLoader.cpp:633
#4	0x03c45ef0 in WebCore::FrameLoader::transitionToCommitted at FrameLoader.cpp:2900
#5	0x03c46572 in WebCore::FrameLoader::commitProvisionalLoad at FrameLoader.cpp:2825
#6	0x03b0d030 in WebCore::DocumentLoader::commitIfReady at DocumentLoader.cpp:320
#7	0x03b0d061 in WebCore::DocumentLoader::commitLoad at DocumentLoader.cpp:340
#8	0x03b0d0ee in WebCore::DocumentLoader::receivedData at DocumentLoader.cpp:354
#9	0x03c3a815 in WebCore::FrameLoader::receivedData at FrameLoader.cpp:2472
#10	0x03fa8aee in WebCore::MainResourceLoader::addData at MainResourceLoader.cpp:143
#11	0x0413b657 in WebCore::ResourceLoader::didReceiveData at ResourceLoader.cpp:247
#12	0x03fa8146 in WebCore::MainResourceLoader::didReceiveData at MainResourceLoader.cpp:352
#13	0x0413aae8 in WebCore::ResourceLoader::didReceiveData at ResourceLoader.cpp:397

[Sam Weinig]

AP, yeah, we should not be clearing event listeners for safe to transition cases like this.

[Dmitry Titov]

Created attachment 39033 [details]
Proposed patch

The proposed fix makes WebKit behavior match both html5 spec and FF 3.5. Also adds tests.

Now we don't remove event listeners on DOMWindow in case of transitioning from empty temporary document (created by FrameLoader::init()) to final document. This is accomplished by the check in FrameLoader::stopLoading that checks for the safe transiotion (similar to the one in FrameLoader::begin(..) that prevents erasing window properties).

Another place that was removing all the event handlers was Document::clear(), invoked from Document::implicitOpen() which is called in 2 cases: when transition document is replaced with loaded content (case of this bug) and when document.open() is used from JS. In latter case we actually need to clean event handlers - if only to avoid firing 'load' upon subsequent document.close(), or things will fail on the web. So I've split clear() and moved clearing handlers code to Document::open(..) and the rest to implicitOpen() and removed the Document::clear() at all.

Also FrameLoader::closeURL() was called 2 times during regular navigation - from transitionToCommitted and then immediately after it returns to commitProvisionalLoad via didOpenURL. The problem with that is closeURL calls stopLoading and that tries to fire unload again - so the old code, by blowing all the event handlers,  was making unload fire once, so it was ok to call it twice. Now that we don't always blow the event handlers, that would cause 2 unload events fired - so I move it out of didOpenURL.

[Dmitry Titov]

More background info: this is the change that fixed related (but different) issue before: http://trac.webkit.org/changeset/25783. It introduced the isSecureTransition() method to see when properties on window have to be kept across this specific navigation type.

[Adam Barth]

I haven't reviewed the tests, but the code changes appear reasonable.  We should have someone else look at them in detail, however, because this is very intricate code.

[Adam Barth]

Comment on attachment 39033 [details]
Proposed patch

I'm moderately confident that this change is correct, and other folks had a month to comment.

[WebKit Commit Bot]

Comment on attachment 39033 [details]
Proposed patch

Clearing flags on attachment: 39033

Committed r49507: <http://trac.webkit.org/changeset/49507>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.