Crash in DOMWindow::setLocation() due to null m_frame

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

Original report at http://code.google.com/p/chromium/issues/detail?id=68077

Opening a new window, setting a timeout to close it, and setting a later timeout to set window.location will cause chromium to crash.  DOMWindow::setLocation() null checks the Frames for the activeWindow and firstWindow parameters it receives, but it uses its own m_frame without null-checking it.

For reasons that aren't clear to me, JSC never calls JSDOMWindow::setLocation() in this example, so this crash doesn't show up in Safari.  I think a fix for this still goes in DOMWindow, however, because if DOMWindow is going to use its own m_frame and it isn't guaranteed to be non-null, we should probably be checking it.  If others disagree, we can kick this fix into the v8 bindings instead.

[Nate Chapin]

Created attachment 79525 [details]
patch + test

[Adam Barth]

Comment on attachment 79525 [details]
patch + test

View in context: https://bugs.webkit.org/attachment.cgi?id=79525&action=review

I don't love the test.  Too many timeouts.  This test takes 1 whole second to run!  There's got to be a better way.  For example, you can have the newly opened window postMessage back to the original page to know that it's time to close it.

> LayoutTests/fast/dom/Window/Location/set-location-after-close.html:14
> +setTimeout('w.location = \'\'; if (window.layoutTestController) layoutTestController.notifyDone()', 1000);

\'\' -> ""

[Alexey Proskuryakov]

This test passes in Firefox, right? And how does window.location come back - is the setter really ignored?

[Nate Chapin]

(In reply to comment #2)
> (From update of attachment 79525 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79525&action=review
> 
> I don't love the test.  Too many timeouts.  This test takes 1 whole second to run!  There's got to be a better way.  For example, you can have the newly opened window postMessage back to the original page to know that it's time to close it.
> 
> > LayoutTests/fast/dom/Window/Location/set-location-after-close.html:14
> > +setTimeout('w.location = \'\'; if (window.layoutTestController) layoutTestController.notifyDone()', 1000);
> 
> \'\' -> ""

Yeah, I just modified the reduction provided in the Chromium bug to make it a layout test.  I'll see if this can be reproed more sanely.



(In reply to comment #3)
> This test passes in Firefox, right? And how does window.location come back - is the setter really ignored?

I'll double-check other browsers on whatever test I end up with.

As for the setter being ignored, I set a breakpoint in JSDOMWindow::setLocation() and never saw it break there, so it really does seem that at least the custom binding isn't getting called.  I don't know enough about the guts of JSC to understand how this could happen.

[Nate Chapin]

Created attachment 79634 [details]
Test using postMessage instead of setTimeout

Thanks for the postMessage suggestion, Adam. :)

This test behaves the same way as the old test on existing Chromium builds, and works correctly in FF as well.

[Alexey Proskuryakov]

Did you check what Firefox returns as window.location value after setting?

[Nate Chapin]

(In reply to comment #6)
> Did you check what Firefox returns as window.location value after setting?

It appears FF throws an error trying to set window.location.  window.location is inaccessible (for getting or setting) as soon as the window.close() call returns.

[Alexey Proskuryakov]

Between the different behavior in Firefox and unexplained behavior in Safari, it's not entirely clear why the patch implements the right thing.

[Adam Barth]

(In reply to comment #8)
> Between the different behavior in Firefox and unexplained behavior in Safari, it's not entirely clear why the patch implements the right thing.

Generally speaking, the behavior for Frame-less objects is very inconsistent between browsers.  This patch follows our usual approach.  At some point in the future, we might want to make an effort to converge browser behavior in this area.  We tried discussing this issue a year or so ago, and folks had particular reasons for liking their individual approaches and not overly much incentive to align.  In any case, that's a topic for another day.

[Adam Barth]

Comment on attachment 79634 [details]
Test using postMessage instead of setTimeout

View in context: https://bugs.webkit.org/attachment.cgi?id=79634&action=review

> LayoutTests/fast/dom/Window/Location/resources/set-location-after-close-new-window.html:2
> +<body onunload="opener.postMessage('closed', '*');">

I wonder if this is racy.  It's probably ok.

[Alexey Proskuryakov]

> We tried discussing this issue a year or so ago, 

Do you at least have a link to that discussion? This bug is not in a very good form now, I'm not sure why you had to hurry with r+/cq+.

[Adam Barth]

(In reply to comment #11)
> > We tried discussing this issue a year or so ago, 
> 
> Do you at least have a link to that discussion?

It was either in the TC39 working group or the HTML working group.  I don't recall off-hand.  Maciej didn't want to implement what IE is doing because he was worried about adding a branch to every JS function call.  Someone else didn't want to implement what we do because it lets you execute JavaScript in a strange Frame-less state that they were worried would lead to security bugs.

> This bug is not in a very good form now, I'm not sure why you had to hurry with r+/cq+.

This patch is adding a null check like hundreds of other null checks we have in the code.  Certainly not crashing is better behavior than crashing.  We can file a follow-up bug about harmonizing our Frame-less execution behavior with other browsers if you like, but that's going to take a lot longer than we want to wait before fixing this crash.

[Alexey Proskuryakov]

What I'm worried about is that we didn't capture information about what happens in IE, or even in Safari in this bug. The description says:

> For reasons that aren't clear to me, JSC never calls JSDOMWindow::setLocation() in this example

Maybe there is protection against this that's just simply broken in v8? It's not clear that the fix should be so local.

[WebKit Commit Bot]

Comment on attachment 79634 [details]
Test using postMessage instead of setTimeout

Clearing flags on attachment: 79634

Committed r76303: <http://trac.webkit.org/changeset/76303>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

> Maybe there is protection against this that's just simply broken in v8? It's not clear that the fix should be so local.

Generally speaking, every method on DOMWindow that can be called by JavaScript needs to be prepared to be called with a null frame.  We commonly forget because the case doesn't occur very often, but that's one of the things I was trying to convey in this diagram:

http://webkit.org/coding/major-objects.html