Summary: | Crash using the new WKBundleDOMWindowExtensions APIs | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jessie Berlin <jberlin> | ||||||
Component: | Page Loading | Assignee: | Jessie Berlin <jberlin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, beidson, dglazkov, japhet, jberlin, webkit.review.bot | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 85891 | ||||||||
Attachments: |
|
Description
Jessie Berlin
2012-05-08 09:12:15 PDT
Created attachment 141020 [details]
Patch
Comment on attachment 141020 [details]
Patch
Assuming all green EWS, a big r+
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. Comment on attachment 141020 [details] Patch Attachment 141020 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12654735 (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. Created attachment 141045 [details]
Patch with Chromium build fix and addressing some comments from Darin
Comment on attachment 141045 [details] Patch with Chromium build fix and addressing some comments from Darin Committed in http://trac.webkit.org/changeset/116595 |