Bug 52769 - Crash in DOMWindow::setLocation() due to null m_frame
Summary: Crash in DOMWindow::setLocation() due to null m_frame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-19 17:02 PST by Nate Chapin
Modified: 2011-01-20 16:37 PST (History)
4 users (show)

See Also:


Attachments
patch + test (3.24 KB, patch)
2011-01-19 17:11 PST, Nate Chapin
abarth: review+
Details | Formatted Diff | Diff
Test using postMessage instead of setTimeout (4.16 KB, patch)
2011-01-20 12:33 PST, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2011-01-19 17:02:33 PST
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.
Comment 1 Nate Chapin 2011-01-19 17:11:21 PST
Created attachment 79525 [details]
patch + test
Comment 2 Adam Barth 2011-01-19 17:35:49 PST
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);

\'\' -> ""
Comment 3 Alexey Proskuryakov 2011-01-19 21:58:18 PST
This test passes in Firefox, right? And how does window.location come back - is the setter really ignored?
Comment 4 Nate Chapin 2011-01-20 09:42:50 PST
(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.
Comment 5 Nate Chapin 2011-01-20 12:33:07 PST
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.
Comment 6 Alexey Proskuryakov 2011-01-20 12:43:38 PST
Did you check what Firefox returns as window.location value after setting?
Comment 7 Nate Chapin 2011-01-20 13:01:21 PST
(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.
Comment 8 Alexey Proskuryakov 2011-01-20 14:19:11 PST
Between the different behavior in Firefox and unexplained behavior in Safari, it's not entirely clear why the patch implements the right thing.
Comment 9 Adam Barth 2011-01-20 15:39:00 PST
(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.
Comment 10 Adam Barth 2011-01-20 15:40:01 PST
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.
Comment 11 Alexey Proskuryakov 2011-01-20 15:43:55 PST
> 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+.
Comment 12 Adam Barth 2011-01-20 15:55:32 PST
(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.
Comment 13 Alexey Proskuryakov 2011-01-20 16:05:11 PST
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.
Comment 14 WebKit Commit Bot 2011-01-20 16:05:43 PST
Comment on attachment 79634 [details]
Test using postMessage instead of setTimeout

Clearing flags on attachment: 79634

Committed r76303: <http://trac.webkit.org/changeset/76303>
Comment 15 WebKit Commit Bot 2011-01-20 16:05:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Adam Barth 2011-01-20 16:37:01 PST
> 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