WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
185712
NavigationAction should not hold a strong reference to a Document
https://bugs.webkit.org/show_bug.cgi?id=185712
Summary
NavigationAction should not hold a strong reference to a Document
Simon Fraser (smfr)
Reported
2018-05-16 21:39:49 PDT
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.
Attachments
[Patch] Remove NavigationAction::sourceDocument()
(12.44 KB, patch)
2018-05-22 16:20 PDT
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
[Patch] Remove NavigationAction::sourceDocument()
(12.43 KB, patch)
2018-05-22 17:33 PDT
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2
(2.97 MB, application/zip)
2018-05-22 18:35 PDT
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews102 for mac-sierra
(2.32 MB, application/zip)
2018-05-22 18:40 PDT
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews121 for ios-simulator-wk2
(2.30 MB, application/zip)
2018-05-22 19:18 PDT
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews113 for mac-sierra
(3.04 MB, application/zip)
2018-05-22 19:21 PDT
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews206 for win-future
(12.75 MB, application/zip)
2018-05-22 20:24 PDT
,
EWS Watchlist
no flags
Details
Patch
(15.29 KB, patch)
2018-05-24 13:20 PDT
,
Daniel Bates
bfulgham
: review+
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2018-05-16 21:40:16 PDT
<
rdar://problem/40320916
>
Simon Fraser (smfr)
Comment 2
2018-05-16 22:03:57 PDT
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)
Comment 3
2018-05-16 22:04:41 PDT
This bug means that when you're browsing, we'll keep one more document alive than we did before.
Simon Fraser (smfr)
Comment 4
2018-05-16 22:06:48 PDT
NavigationAction also retains the Event, which itself can retain EventTargets which are DOM nodes which can also entrain the entire document.
Ryosuke Niwa
Comment 5
2018-05-21 13:10:57 PDT
Is this a regression?
Chris Dumez
Comment 6
2018-05-21 13:14:24 PDT
I am not even sure NavigationAction::event() is ever used?
Daniel Bates
Comment 7
2018-05-21 13:32:38 PDT
(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
Comment 8
2018-05-21 13:33:08 PDT
(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
Comment 9
2018-05-21 13:33:33 PDT
(In reply to Chris Dumez from
comment #6
)
> I am not even sure NavigationAction::event() is ever used?
Can you please elaborate?
Daniel Bates
Comment 10
2018-05-22 16:20:30 PDT
Created
attachment 341045
[details]
[Patch] Remove NavigationAction::sourceDocument()
EWS Watchlist
Comment 11
2018-05-22 16:24:01 PDT
Comment hidden (obsolete)
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
Comment 12
2018-05-22 17:33:22 PDT
Created
attachment 341051
[details]
[Patch] Remove NavigationAction::sourceDocument()
EWS Watchlist
Comment 13
2018-05-22 17:36:10 PDT
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 14
2018-05-22 18:35:47 PDT
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
Comment 15
2018-05-22 18:35:48 PDT
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 16
2018-05-22 18:40:19 PDT
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
Comment 17
2018-05-22 18:40:21 PDT
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 18
2018-05-22 19:18:20 PDT
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
Comment 19
2018-05-22 19:18:21 PDT
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 20
2018-05-22 19:21:00 PDT
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
Comment 21
2018-05-22 19:21:01 PDT
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 22
2018-05-22 20:24:47 PDT
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
Comment 23
2018-05-22 20:24:58 PDT
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
Comment 24
2018-05-24 13:20:49 PDT
Created
attachment 341220
[details]
Patch
EWS Watchlist
Comment 25
2018-05-24 13:22:10 PDT
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 26
2018-05-24 13:45:51 PDT
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 27
2018-05-25 16:45:18 PDT
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
Comment 28
2018-05-25 16:49:24 PDT
Committed
r232216
: <
https://trac.webkit.org/changeset/232216
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug