Bug 85888 – Crash using the new WKBundleDOMWindowExtensions APIs

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Jessie Berlin]

>  1 com.apple.WebCore              0x10f590504 WebCore::DOMWindowProperty** WTF::HashTable<WebCore::DOMWindowProperty*, WebCore::DOMWindowProperty*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::DOMWindowProperty*>, WTF::HashTraits<WebCore::DOMWindowProperty*>, WTF::HashTraits<WebCore::DOMWindowProperty*> >::lookup<WTF::IdentityHashTranslator<WTF::PtrHash<WebCore::DOMWindowProperty*> >, WebCore::DOMWindowProperty*>(WebCore::DOMWindowProperty* const&) + 0xb4
   2 com.apple.WebCore              0x10f5903f8 WTF::HashSet<WebCore::DOMWindowProperty*, WTF::PtrHash<WebCore::DOMWindowProperty*>, WTF::HashTraits<WebCore::DOMWindowProperty*> >::remove(WebCore::DOMWindowProperty* const&) + 0x28
   3 com.apple.WebCore              0x10f58fff9 WebCore::DOMWindow::unregisterProperty(WebCore::DOMWindowProperty*) + 0x19
   4 com.apple.WebCore              0x10f590f55 WebCore::DOMWindowExtension::~DOMWindowExtension() + 0x25
   5 com.apple.WebCore              0x10f590f11 WebCore::DOMWindowExtension::~DOMWindowExtension() + 0x11
   6 com.apple.WebKit2              0x10e95beca WebKit::InjectedBundleDOMWindowExtension::~InjectedBundleDOMWindowExtension() + 0x50
   7 com.apple.WebKit2              0x10e95be61 WebKit::InjectedBundleDOMWindowExtension::~InjectedBundleDOMWindowExtension() + 0x11

A number of things may be/are going wrong here:

1. WKBundlePageWillDestroyGlobalObjectForDOMWindowExtensionCallback is only being invoked when the WKPage is destroyed, and then only for the child frames (among other things, CachedFrame does not ever invoke willDetachPage and therefore never results in this callback being invoked).
2. The DOMWindow may be going away before the DOMWindowExtension is destroyed, but it still holds a reference to m_disconnectedDOMWindow and attempts to unregister from it.
3. Even if CachedFrames did attempt to invoked the callback, the page would be gone so it wouldn't be possible for the injected bundle to dispatch the callback on the page client.

Brady and I are working on a fix.

<rdar://problem/11349796>

[Jessie Berlin]

Created attachment 141020 [details]
Patch

[Brady Eidson]

Comment on attachment 141020 [details]
Patch

Assuming all green EWS, a big r+

[Darin Adler]

Comment on attachment 141020 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:2070
> +    if (DOMWindow* domWindow = this->domWindow())
> +        domWindow->willDetachDocumentFromFrame();

I’d just have named this local variable “window”.

> Source/WebCore/loader/appcache/DOMApplicationCache.cpp:70
> +    ApplicationCacheHost* cacheHost = applicationCacheHost();
> +    if (cacheHost)
> +        cacheHost->setDOMApplicationCache(0);

I’d suggest putting this definitions inside the if statement. I see that the other functions above aren’t doing this, but they should.

> Source/WebCore/page/DOMWindow.cpp:488
>      copyToVector(m_properties, properties);

This existing code needs a why comment, explaining both why copying the vector is needed, and why it’s safe to copy a vector of raw pointers to reference counted objects. I assume the raw pointers are safe because the function can remove the property from the vector, but it’s guaranteed to only be the property we actually called the function on.

> Source/WebCore/page/DOMWindow.cpp:498
> +    Vector<DOMWindowProperty*> properties;
> +    copyToVector(m_properties, properties);
> +    for (size_t i = 0; i < properties.size(); ++i)
> +        properties[i]->willDestroyGlobalObjectInFrame();

To avoid writing this loop many times we could use a function template. An example of how to do this is <http://trac.webkit.org/changeset/116254/trunk/Source/WebCore/page/ContentSecurityPolicy.cpp>.

> Source/WebCore/page/Frame.cpp:265
> +    // Prepare for destruction now, so any onUnload handlers get run and the DOMWindow is notified

What this comment calls onUnload handlers are actually “unload event handlers”.

> Source/WebCore/page/Frame.cpp:267
> +    // - if we wait until the view is destroyed, then things won't be hooked up enough for these
> +    // calls to work.

I think this is just a second sentence, so you should remove the hyphen and add a period and capital letter.

[WebKit Review Bot]

Comment on attachment 141020 [details]
Patch

Attachment 141020 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12654735

[Jessie Berlin]

(In reply to comment #3)
> (From update of attachment 141020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141020&action=review
> 
> > Source/WebCore/dom/Document.cpp:2070
> > +    if (DOMWindow* domWindow = this->domWindow())
> > +        domWindow->willDetachDocumentFromFrame();
> 
> I’d just have named this local variable “window”.

Fixed.

> 
> > Source/WebCore/loader/appcache/DOMApplicationCache.cpp:70
> > +    ApplicationCacheHost* cacheHost = applicationCacheHost();
> > +    if (cacheHost)
> > +        cacheHost->setDOMApplicationCache(0);
> 
> I’d suggest putting this definitions inside the if statement. I see that the other functions above aren’t doing this, but they should.

Done for all the other functions that I was already touching.

> 
> > Source/WebCore/page/DOMWindow.cpp:488
> >      copyToVector(m_properties, properties);
> 
> This existing code needs a why comment, explaining both why copying the vector is needed, and why it’s safe to copy a vector of raw pointers to reference counted objects. I assume the raw pointers are safe because the function can remove the property from the vector, but it’s guaranteed to only be the property we actually called the function on.

m_properties is a HashSet<DOMWindowProperty*>.

> 
> > Source/WebCore/page/DOMWindow.cpp:498
> > +    Vector<DOMWindowProperty*> properties;
> > +    copyToVector(m_properties, properties);
> > +    for (size_t i = 0; i < properties.size(); ++i)
> > +        properties[i]->willDestroyGlobalObjectInFrame();
> 
> To avoid writing this loop many times we could use a function template. An example of how to do this is <http://trac.webkit.org/changeset/116254/trunk/Source/WebCore/page/ContentSecurityPolicy.cpp>.

I think I am going to have to skip this for now in the interest of time.

> 
> > Source/WebCore/page/Frame.cpp:265
> > +    // Prepare for destruction now, so any onUnload handlers get run and the DOMWindow is notified
> 
> What this comment calls onUnload handlers are actually “unload event handlers”.

Fixed.

> 
> > Source/WebCore/page/Frame.cpp:267
> > +    // - if we wait until the view is destroyed, then things won't be hooked up enough for these
> > +    // calls to work.
> 
> I think this is just a second sentence, so you should remove the hyphen and add a period and capital letter.

Fixed.

Chromium build-fix patch with these fixes coming shortly.

[Jessie Berlin]

Created attachment 141045 [details]
Patch with Chromium build fix and addressing some comments from Darin

[Jessie Berlin]

Comment on attachment 141045 [details]
Patch with Chromium build fix and addressing some comments from Darin

Committed in http://trac.webkit.org/changeset/116595