Bug 29832 - Crash in DOMWindow::clearTimeout etc when DOMWindow is not connected to frame
: Crash in DOMWindow::clearTimeout etc when DOMWindow is not connected to frame
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-09-28 15:41 PST by
Modified: 2011-05-17 10:30 PST (History)


Attachments
Test file reproducing the crash in Safari (574 bytes, text/html)
2009-10-02 22:47 PST, Dmitry Titov
no flags Details
Crash dump (54.40 KB, text/plain)
2009-10-05 10:53 PST, Dmitry Titov
no flags Details
Proposed fix. (1.35 KB, patch)
2009-10-05 10:58 PST, Dmitry Titov
fishd: review-
Review Patch | Details | Formatted Diff | Diff
Updated patch. (1.36 KB, patch)
2009-10-05 23:54 PST, Dmitry Titov
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch. (1.36 KB, patch)
2009-10-05 23:56 PST, Dmitry Titov
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-09-28 15:41:47 PST
The Chromium port has been seeing some crashes due to DOMWindow::clearTimeout and DOMWindow::clearInterval passing a null Document* as the ScriptExecutionContext into DOMTimer::removeById().

I'd like to add a null check and early return to DOMTimer::removeById(). Since ScriptExecutionContexts should delete all related timers when they are destructed, we shouldn't need to do anything if told to remove a timer from a null ScriptExecutionContext.

In addition, the attempted fix in the V8 bindings needs to be removed, as it was placed in custom bindings that aren't actually in use.
------- Comment #1 From 2009-09-28 15:50:36 PST -------
I think it would be much better to find out why null pointers are being passed then to just add a work-around.  Has there been investigation into the cause?
------- Comment #2 From 2009-09-28 15:58:02 PST -------
Some, but nothing definite has been concluded.  It appears based on the fact that DOMWindow::document() is returning 0 and not failing any assertions, it seems either DOMWindow:m_frame is getting zeroed, or m_frame->domWindow() is pointing to a different DOMWindow than this.

Unfortuantely, I don't know how to deterministically reproduce the crash, but I'll do some more digging before submitting a patch to this bug.
------- Comment #3 From 2009-09-28 16:02:31 PST -------
A null document here is probably indicative of a navigation.  I would guess that the timers are not getting correctly suspended in some case.
------- Comment #4 From 2009-10-02 22:47:00 PST -------
Created an attachment (id=40564) [details]
Test file reproducing the crash in Safari

Interesting. I found one way to reproduce it - see attached file. Crashes Safari with WebKit ToT.

I found this repro looking  for incorrect suspend/resume of timers per Sam's suggestion. Didn't find those, but found at least one where timer in one window refers to another window, and PageCahe expiration starts to close cached pages. The DOMWindow.m_frame is NULL and JS continues to call window.clearTimeout.

I'm weak in JSC details, but here is what I see in the debugger:
Apparently, DOMWindow w/o frame happens as result of navigation or window close - in these cases the  JSDOMWindow::getOwnPropertySlot starts to tell JS that window only has 'close' and 'closed', and throws TypeError otherwise. JSDOMWindow::getOwnPropertySlot is not called all the time, the result seems to be cached (not yet sure how). So page destruction and navigation code paths use ScriptController::clearWindowShell() and this seems to have an effect of re-creating JSDOMWindow wrapper and invalidating the caches - so the properties/methods get re-resolved.

Adding ScriptController::clearWindowShell() to Frame destructor right after it disconnects from the DOMWindow (so if JSDOMWindowShell is still referenced by some script in other pages, it'll start act as 'closed' window) fixes the crash...
------- Comment #5 From 2009-10-05 10:53:07 PST -------
Created an attachment (id=40643) [details]
Crash dump
------- Comment #6 From 2009-10-05 10:58:40 PST -------
Created an attachment (id=40644) [details]
Proposed fix.

I think this might be a fix. Clearing WindowShell after Frame disconnects from DOMWindow (and renders it unable to execute most JS callbacks) will clear global script data and cause re-resolution of properties. I can't think of a DRT test for that, the one I have takes ~10 seconds to run which is too long.
------- Comment #7 From 2009-10-05 23:32:08 PST -------
(From update of attachment 40644 [details])
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Crash Crash in DOMWindow::clearTimeout etc when DOMWindow is not connected to frame.

extra "Crash"


> +        https://bugs.webkit.org/show_bug.cgi?id=29832
> +
> +		Need to make sure the script caches are reset when frame gets disconnected from still-alive DOMWindow.
> +		This will prevent JS from calling DOMWindow methods that can not be completed w/o the frame.

oops ^^^ tabs need to be changed to spaces
------- Comment #8 From 2009-10-05 23:54:47 PST -------
Created an attachment (id=40695) [details]
Updated patch.

Oops. Updated :-) Thanks for looking!
------- Comment #9 From 2009-10-05 23:56:01 PST -------
Created an attachment (id=40696) [details]
Updated patch.
------- Comment #10 From 2009-10-06 09:57:49 PST -------
Sam is your man here.
------- Comment #11 From 2009-10-07 18:00:58 PST -------
I hope Sam will have time to take a look. Or anybody else who feel ok with it.

Obviously, alternative fix would be to simply check for m_frame being NULL in DOMWindow set/clearTimeout, set/clearInterval methods - consistently with another couple dozen methods on DOMWindow that already do that. Perhaps that would be easier to review too.

I thought it would be nice to avoid conditions when JS is using DOMWindow which is disconnected. But if the fix looks risky, we could just check for null :-)
------- Comment #12 From 2009-10-07 19:00:19 PST -------
> I thought it would be nice to avoid conditions when JS is using DOMWindow which
> is disconnected. But if the fix looks risky, we could just check for null :-)

Drive-by comment: we can't avoid JS playing with detached DOMWindows.
------- Comment #13 From 2009-10-07 20:23:41 PST -------
(In reply to comment #12)
> Drive-by comment: we can't avoid JS playing with detached DOMWindows.

I think we can - with WidowShell->JSDOMWindow->DOMWindow triplet we should be able to have a check in a single place (and this is what we have now already at the level of JSDOMWindow custom getters)
------- Comment #14 From 2009-10-18 22:39:57 PST -------
(From update of attachment 40696 [details])
This change is fine.  I'm sad that we can't test it, but I believe you when you say it's requires a 10 second test.
------- Comment #15 From 2009-10-19 08:48:09 PST -------
(From update of attachment 40696 [details])
Let commit bot land it
------- Comment #16 From 2009-10-19 09:00:28 PST -------
(From update of attachment 40696 [details])
Clearing flags on attachment: 40696

Committed r49786: <http://trac.webkit.org/changeset/49786>
------- Comment #17 From 2009-10-19 09:00:32 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #18 From 2009-10-19 15:41:32 PST -------
(In reply to comment #14)
> This change is fine.  I'm sad that we can't test it, but I believe you when you
> say it's requires a 10 second test.

Normally in such cases we put the test into WebCore/manual-tests with instructions on how to run it.
------- Comment #19 From 2009-10-19 15:51:25 PST -------
(In reply to comment #18)
> (In reply to comment #14)
> > This change is fine.  I'm sad that we can't test it, but I believe you when you
> > say it's requires a 10 second test.
> 
> Normally in such cases we put the test into WebCore/manual-tests with
> instructions on how to run it.

Good point! The bug and a patch with manual test is coming.
------- Comment #20 From 2011-05-15 16:27:28 PST -------
> Good point! The bug and a patch with manual test is coming.

Did you land a manual test?
------- Comment #21 From 2011-05-15 16:32:20 PST -------
(In reply to comment #12)
> > I thought it would be nice to avoid conditions when JS is using DOMWindow which
> > is disconnected. But if the fix looks risky, we could just check for null :-)
> 
> Drive-by comment: we can't avoid JS playing with detached DOMWindows.

FWIW, it turned out that we can't avoid JS playing with detached DOMWindows, and Alexey had to re-fix this bug with a null check. See bug 33815.
------- Comment #22 From 2011-05-15 16:36:01 PST -------
Also, the fix here caused this performance problem: https://bugs.webkit.org/show_bug.cgi?id=33815.
------- Comment #23 From 2011-05-15 18:14:22 PST -------
> Also, the fix here caused this performance problem: https://bugs.webkit.org/show_bug.cgi?id=33815.

Geoff, is that the right bug#?
------- Comment #24 From 2011-05-16 15:25:15 PST -------
(In reply to comment #23)
> > Also, the fix here caused this performance problem: https://bugs.webkit.org/show_bug.cgi?id=33815.
> 
> Geoff, is that the right bug#?

I believe the performance issue (which Geoff fixed) was <https://bugs.webkit.org/show_bug.cgi?id=59699>.
------- Comment #25 From 2011-05-16 20:33:08 PST -------
Oops! Pratik is right -- the performance regression was https://bugs.webkit.org/show_bug.cgi?id=59699.
------- Comment #26 From 2011-05-17 10:30:34 PST -------
The manual test for this crash was landed in http://trac.webkit.org/changeset/49824

Great to see the better fix!