Bug 185712 – NavigationAction should not hold a strong reference to a Document

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

[Simon Fraser (smfr)]

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.

[Radar WebKit Bug Importer]

<rdar://problem/40320916>

[Simon Fraser (smfr)]

WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction uses this sourceDocument to get at URL and originating frameID. Maybe NavigationAction should store those instead of retaining the sourceDocument.

[Simon Fraser (smfr)]

This bug means that when you're browsing, we'll keep one more document alive than we did before.

[Simon Fraser (smfr)]

NavigationAction also retains the Event, which itself can retain EventTargets which are DOM nodes which can also entrain the entire document.

[Ryosuke Niwa]

Is this a regression?

[Chris Dumez]

I am not even sure NavigationAction::event() is ever used?

[Daniel Bates]

(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.

[Daniel Bates]

(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 ...

[Daniel Bates]

(In reply to Chris Dumez from comment #6)
> I am not even sure NavigationAction::event() is ever used?

Can you please elaborate?

[Daniel Bates]

Created attachment 341045 [details]
[Patch] Remove NavigationAction::sourceDocument()

[EWS Watchlist]

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.

[Daniel Bates]

Created attachment 341051 [details]
[Patch] Remove NavigationAction::sourceDocument()

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Daniel Bates]

Created attachment 341220 [details]
Patch

[EWS Watchlist]

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.

[Brent Fulgham]

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.

[Daniel Bates]

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.

[Daniel Bates]

Committed r232216: <https://trac.webkit.org/changeset/232216>