| Summary: | NavigationAction should not hold a strong reference to a Document | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> |
| Component: | Page Loading | Assignee: | Daniel Bates <dbates> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | achristensen, beidson, bfulgham, cdumez, dbates, ews-watchlist, japhet, mitz, rniwa, sam, simon.fraser, webkit-bug-importer |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Attachments: | |||
WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction uses this sourceDocument to get at URL and originating frameID. Maybe NavigationAction should store those instead of retaining the sourceDocument. This bug means that when you're browsing, we'll keep one more document alive than we did before. NavigationAction also retains the Event, which itself can retain EventTargets which are DOM nodes which can also entrain the entire document. Is this a regression? I am not even sure NavigationAction::event() is ever used? (In reply to Ryosuke Niwa from comment #5) > Is this a regression? No, this is not a regression per comment 4. We have exacerbated the memory issue following the fix for bug #165160. (In reply to Daniel Bates from comment #7) > (In reply to Ryosuke Niwa from comment #5) > > Is this a regression? > > No, this is not a regression per comment 4. We have exacerbated the memory > issue following the fix for bug #165160. * exacerbated the occurrence of ... (In reply to Chris Dumez from comment #6) > I am not even sure NavigationAction::event() is ever used? Can you please elaborate? Created attachment 341045 [details]
[Patch] Remove NavigationAction::sourceDocument()
Attachment 341045 [details] did not pass style-queue:
ERROR: Source/WebCore/loader/NavigationAction.cpp:42: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 341051 [details]
[Patch] Remove NavigationAction::sourceDocument()
Attachment 341051 [details] did not pass style-queue:
ERROR: Source/WebCore/loader/NavigationAction.cpp:42: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 341051 [details] [Patch] Remove NavigationAction::sourceDocument() Attachment 341051 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7771476 New failing tests: http/tests/navigation/window-open-redirect-and-remove-opener.html Created attachment 341055 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 341051 [details] [Patch] Remove NavigationAction::sourceDocument() Attachment 341051 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/7771518 New failing tests: http/tests/navigation/window-open-redirect-and-remove-opener.html Created attachment 341056 [details]
Archive of layout-test-results from ews102 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 341051 [details] [Patch] Remove NavigationAction::sourceDocument() Attachment 341051 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7771580 New failing tests: http/tests/navigation/window-open-redirect-and-remove-opener.html Created attachment 341059 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 341051 [details] [Patch] Remove NavigationAction::sourceDocument() Attachment 341051 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/7771567 New failing tests: http/tests/navigation/window-open-redirect-and-remove-opener.html Created attachment 341060 [details]
Archive of layout-test-results from ews113 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 341051 [details] [Patch] Remove NavigationAction::sourceDocument() Attachment 341051 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7772175 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html http/tests/security/canvas-remote-read-remote-video-redirect.html http/tests/navigation/window-open-redirect-and-remove-opener.html Created attachment 341065 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 341220 [details]
Patch
Attachment 341220 [details] did not pass style-queue:
ERROR: Source/WebCore/loader/NavigationAction.cpp:42: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 341220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341220&action=review Looks good! r=me. > Source/WebCore/loader/NavigationAction.cpp:40 > +NavigationAction::Requester::Requester(Document& document) Could this be 'const Document&'? Or does one of the frame methods mutate the document or frame? > Source/WebCore/loader/NavigationAction.cpp:57 > + return url.isBlankURL() || url.protocolIsData() || (url.protocolIsBlob() && document.securityOrigin().canRequest(url)); I don't think this whitespace change is helpful. In fact, it makes it more difficult to easily see which predicate is causing an early return. > Source/WebCore/loader/NavigationAction.h:60 > + Requester(Document&); const Document& > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:851 > + originatingFrameInfoData.securityOrigin = SecurityOrigin::create(requester.url())->data(); Was the sourceDocument's SecurityOrigin member always computed dynamically, or was it cached? SecurityOrigin can be expensive to compute. Comment on attachment 341220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341220&action=review >> Source/WebCore/loader/NavigationAction.cpp:40 >> +NavigationAction::Requester::Requester(Document& document) > > Could this be 'const Document&'? Or does one of the frame methods mutate the document or frame? Will change. >> Source/WebCore/loader/NavigationAction.cpp:57 >> + return url.isBlankURL() || url.protocolIsData() || (url.protocolIsBlob() && document.securityOrigin().canRequest(url)); > > I don't think this whitespace change is helpful. In fact, it makes it more difficult to easily see which predicate is causing an early return. I am going to commit this as-is. The whitespace seems excessive and quickly looking through our code we typically do not break out disjuncts across multiple lines. >> Source/WebCore/loader/NavigationAction.h:60 >> + Requester(Document&); > > const Document& Will change. >> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:851 >> + originatingFrameInfoData.securityOrigin = SecurityOrigin::create(requester.url())->data(); > > Was the sourceDocument's SecurityOrigin member always computed dynamically, or was it cached? SecurityOrigin can be expensive to compute. Will cache the origin. We actually need to do this for correctness for sandboxed documents. Such documents may have a URL with a file or network scheme (e.g. HTTP) and a unique origin. Committed r232216: <https://trac.webkit.org/changeset/232216> |
NavigationAction is holding references to Documents, which keeps them alive longer than they would normally be. To reproduce: 1. Load simple HMTL file A.html in WK2 MiniBrowser. 2. Load simple HTML file B.html (I dragged the file into the window). 3. Click the Back button. 4. In terminal, run "notifyutil -p org.WebKit.lowMemory" to clear the page cache. At this point, you'd expect to see the Document for B.html be destroyed. But it isn't. The entire DOM for A sticks around. 5. Reload A.html Here B.html's document is finally destroyed: * frame #0: 0x0000000639dec66e WebCore`WebCore::Document::~Document(this=0x0000000651201f40) at Document.cpp:582 frame #1: 0x000000063a188c25 WebCore`WebCore::HTMLDocument::~HTMLDocument(this=0x0000000651201f40) at HTMLDocument.cpp:95 frame #2: 0x000000063a188c45 WebCore`WebCore::HTMLDocument::~HTMLDocument(this=0x0000000651201f40) at HTMLDocument.cpp:95 frame #3: 0x000000063a188ce9 WebCore`WebCore::HTMLDocument::~HTMLDocument(this=0x0000000651201f40) at HTMLDocument.cpp:95 frame #4: 0x0000000639defec0 WebCore`WebCore::Document::decrementReferencingNodeCount(this=0x0000000651201f40) at Document.h:359 frame #5: 0x0000000639defc10 WebCore`WebCore::Document::removedLastRef(this=0x0000000651201f40) at Document.cpp:709 frame #6: 0x0000000639f29cf7 WebCore`WebCore::Node::removedLastRef(this=0x0000000651201f40) at Node.cpp:2480 frame #7: 0x0000000638020bd3 WebCore`WebCore::Node::deref(this=0x0000000651201f40) at Node.h:714 frame #8: 0x00000006392b0701 WebCore`void WTF::derefIfNotNull<WebCore::Document>(ptr=0x0000000651201f40) at RefPtr.h:45 frame #9: 0x00000006392b06c9 WebCore`WTF::RefPtr<WebCore::Document, WTF::DumbPtrTraits<WebCore::Document> >::~RefPtr(this=0x000000064e4f2e68) at RefPtr.h:70 frame #10: 0x00000006392adfe5 WebCore`WTF::RefPtr<WebCore::Document, WTF::DumbPtrTraits<WebCore::Document> >::~RefPtr(this=0x000000064e4f2e68) at RefPtr.h:70 frame #11: 0x000000063a61ec8b WebCore`WebCore::NavigationAction::~NavigationAction(this=0x000000064e4f2e68) at NavigationAction.cpp:42 frame #12: 0x000000063a61ecf5 WebCore`WebCore::NavigationAction::~NavigationAction(this=0x000000064e4f2e68) at NavigationAction.cpp:40 frame #13: 0x000000063a58d1c9 WebCore`WebCore::DocumentLoader::~DocumentLoader(this=0x000000064e4f2400) at DocumentLoader.cpp:181 frame #14: 0x0000000109a6e555 WebKit`WebKit::WebDocumentLoader::~WebDocumentLoader(this=0x000000064e4f2400) at WebDocumentLoader.h:33 frame #15: 0x0000000109a6e3b5 WebKit`WebKit::WebDocumentLoader::~WebDocumentLoader(this=0x000000064e4f2400) at WebDocumentLoader.h:33 frame #16: 0x0000000109a6e3d9 WebKit`WebKit::WebDocumentLoader::~WebDocumentLoader(this=0x000000064e4f2400) at WebDocumentLoader.h:33 frame #17: 0x000000063999c34f WebCore`WTF::RefCounted<WebCore::DocumentLoader>::deref(this=0x000000064e4f2410) const at RefCounted.h:145 frame #18: 0x000000063999c2a5 WebCore`void WTF::derefIfNotNull<WebCore::DocumentLoader>(ptr=0x000000064e4f2400) at RefPtr.h:45 frame #19: 0x000000063999c269 WebCore`WTF::RefPtr<WebCore::DocumentLoader, WTF::DumbPtrTraits<WebCore::DocumentLoader> >::~RefPtr(this=0x00007ffee6b1e3e8) at RefPtr.h:70 frame #20: 0x000000063998d445 WebCore`WTF::RefPtr<WebCore::DocumentLoader, WTF::DumbPtrTraits<WebCore::DocumentLoader> >::~RefPtr(this=0x00007ffee6b1e3e8) at RefPtr.h:70 frame #21: 0x000000063a5e4b29 WebCore`WTF::RefPtr<WebCore::DocumentLoader, WTF::DumbPtrTraits<WebCore::DocumentLoader> >::operator=(this=0x00007fcdd4a11480, optr=0x000000064e4f0000) at RefPtr.h:151 frame #22: 0x000000063a5d9f03 WebCore`WebCore::FrameLoader::setDocumentLoader(this=0x00007fcdd4a11430, loader=0x000000064e4f0000) at FrameLoader.cpp:1819 frame #23: 0x000000063a5e6356 WebCore`WebCore::FrameLoader::transitionToCommitted(this=0x00007fcdd4a11430, cachedPage=0x0000000000000000) at FrameLoader.cpp:2027 This is a problem. NavigationAction needs to not hold strong references to Documents.