Bug 87990 - Crashes unregistering DOMWindowProperties while releasing CachedPages
Summary: Crashes unregistering DOMWindowProperties while releasing CachedPages
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
Keywords: InRadar
Depends on:
Reported: 2012-05-31 10:51 PDT by Brady Eidson
Modified: 2012-05-31 12:58 PDT (History)
0 users

See Also:

Patch v1 (6.11 KB, patch)
2012-05-31 11:03 PDT, Brady Eidson
jberlin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2012-05-31 10:51:11 PDT
Crashes unregistered DOMWindowProperties releasing CachedPages.

I haven't been able to craft a test case to reproduce this, but we know some people are seeing crashes where the following occurs:
-A page with iframes goes in to the page cache
-SOMEHOW, a DOMWindowProperty is created in one of these iframes.  It is registered with the DOMWindow as a property, but it doesn't know that it is a disconnected DOMWindow in the page cache.
-When the cached frame is later destroyed, the DOMWindow tells each of its properties that the cached frame is going away, and the DOMWindowProperty in question doesn't have a disconnected DOMWindow to unregister from.

My proposed patch will involve a pretty straight forward rewrite of the base DOMWindowProperty class that guards against this case by always keeping track of the DOMWindow it has registered with, and only unregistering from that very same DOMWindow.

In radar as <rdar://problem/11544454>
Comment 1 Brady Eidson 2012-05-31 11:03:25 PDT
Created attachment 145114 [details]
Patch v1
Comment 2 Jessie Berlin 2012-05-31 11:18:36 PDT
Comment on attachment 145114 [details]
Patch v1

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

r=me (assuming all the layout tests pass with this version)

> Source/WebCore/page/DOMWindowProperty.cpp:41
> +    // We should fix that.

This needs a FIXME and a bug number.
Comment 3 Brady Eidson 2012-05-31 12:58:29 PDT