Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(15)

Issue 1194003004: Oilpan: enable appcache + move DocumentLoader to the heap. (Closed)

Created:
5 years, 6 months ago by sof
Modified:
5 years, 6 months ago
Reviewers:
oilpan-reviews, haraken
CC:
blink-reviews, tyoshino+watch_chromium.org, arv+blink, vivekg, eae+blinkwatch, Yoav Weiss, blink-reviews-dom_chromium.org, dglazkov+blink, vivekg_samsung, gavinp+loader_chromium.org, Inactive, Nate Chapin, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Oilpan: enable appcache + move DocumentLoader to the heap. DocumentLoader belongs on the heap. References to it are kept by an object that is on the heap (FrameLoader), and DocumentLoader itself has heap references. Along with moving DocumentLoader, have the two appcache objects (ApplicationCache and ApplicationCacheHost) be under Oilpan's control always. R=haraken BUG=340522, 497595 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197576 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197664

Patch Set 1 #

Patch Set 2 : compile fix #

Patch Set 3 : re-intro dispose() (non-oilpan) #

Patch Set 4 : eager finalization #

Patch Set 5 : virtualize detachFromFrame() #

Patch Set 6 : add outer loader protection #

Total comments: 14

Patch Set 7 : introduce detachFromDocumentLoader() #

Total comments: 1

Patch Set 8 : No need to eagerly finalize WebDataSourceImpl #

Patch Set 9 : add explicit DocumentLoader detach to FrameFetchContextTest #

Patch Set 10 : followups; enforce must-detach for document loader + appcache host #

Patch Set 11 : handle nested detaches of DocumentLoader #

Patch Set 12 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+175 lines, -113 lines) Patch
M Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/frame/LocalDOMWindow.h View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/frame/LocalDOMWindow.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/loader/DocumentLoader.h View 1 2 3 4 5 6 5 chunks +15 lines, -12 lines 0 comments Download
M Source/core/loader/DocumentLoader.cpp View 1 2 3 4 5 6 7 8 9 10 13 chunks +40 lines, -20 lines 0 comments Download
M Source/core/loader/EmptyClients.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M Source/core/loader/EmptyClients.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M Source/core/loader/FrameFetchContextTest.cpp View 1 2 3 4 5 6 7 8 5 chunks +14 lines, -3 lines 0 comments Download
M Source/core/loader/FrameLoader.h View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -3 lines 0 comments Download
M Source/core/loader/FrameLoader.cpp View 1 2 3 4 5 6 7 8 9 10 11 11 chunks +24 lines, -19 lines 0 comments Download
M Source/core/loader/FrameLoaderClient.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M Source/core/loader/appcache/ApplicationCache.h View 1 chunk +4 lines, -4 lines 0 comments Download
M Source/core/loader/appcache/ApplicationCache.idl View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/loader/appcache/ApplicationCacheHost.h View 1 2 3 4 5 6 2 chunks +7 lines, -7 lines 0 comments Download
M Source/core/loader/appcache/ApplicationCacheHost.cpp View 1 2 3 4 5 6 6 chunks +32 lines, -32 lines 0 comments Download
M Source/web/FrameLoaderClientImpl.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M Source/web/FrameLoaderClientImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -2 lines 0 comments Download
M Source/web/WebDataSourceImpl.h View 1 2 3 4 5 6 7 3 chunks +5 lines, -1 line 0 comments Download
M Source/web/WebDataSourceImpl.cpp View 1 2 3 4 5 6 7 2 chunks +18 lines, -2 lines 0 comments Download

Messages

Total messages: 28 (7 generated)
sof
please take a look. Long overdue DocumentLoader treatment.
5 years, 6 months ago (2015-06-21 15:46:05 UTC) #2
haraken
Thanks for working on this! https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/DocumentLoader.cpp File Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/DocumentLoader.cpp#newcode117 Source/core/loader/DocumentLoader.cpp:117: m_applicationCacheHost->dispose(); I'm wondering if ...
5 years, 6 months ago (2015-06-22 01:57:59 UTC) #4
sof
https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/appcache/ApplicationCacheHost.cpp File Source/core/loader/appcache/ApplicationCacheHost.cpp (right): https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/appcache/ApplicationCacheHost.cpp#newcode177 Source/core/loader/appcache/ApplicationCacheHost.cpp:177: m_documentLoader = nullptr; On 2015/06/22 01:57:59, haraken wrote: > ...
5 years, 6 months ago (2015-06-22 05:03:46 UTC) #5
haraken
https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/appcache/ApplicationCacheHost.cpp File Source/core/loader/appcache/ApplicationCacheHost.cpp (right): https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/appcache/ApplicationCacheHost.cpp#newcode177 Source/core/loader/appcache/ApplicationCacheHost.cpp:177: m_documentLoader = nullptr; On 2015/06/22 05:03:46, sof wrote: > ...
5 years, 6 months ago (2015-06-22 05:28:56 UTC) #6
sof
On 2015/06/22 05:28:56, haraken wrote: > https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/appcache/ApplicationCacheHost.cpp > File Source/core/loader/appcache/ApplicationCacheHost.cpp (right): > > https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/appcache/ApplicationCacheHost.cpp#newcode177 > ...
5 years, 6 months ago (2015-06-22 05:36:02 UTC) #7
haraken
On 2015/06/22 05:36:02, sof wrote: > On 2015/06/22 05:28:56, haraken wrote: > > > https://codereview.chromium.org/1194003004/diff/100001/Source/core/loader/appcache/ApplicationCacheHost.cpp ...
5 years, 6 months ago (2015-06-22 05:39:21 UTC) #8
sof
On 2015/06/22 05:39:21, haraken wrote: > On 2015/06/22 05:36:02, sof wrote: > > On 2015/06/22 ...
5 years, 6 months ago (2015-06-22 05:56:36 UTC) #9
sof
Given that we have an explicit detach step over DocumentLoader, we can extend that to ...
5 years, 6 months ago (2015-06-22 06:49:18 UTC) #10
sof
https://codereview.chromium.org/1194003004/diff/120001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1194003004/diff/120001/Source/core/dom/Document.cpp#newcode2656 Source/core/dom/Document.cpp:2656: RefPtrWillBeRawPtr<DocumentLoader> documentLoader = m_frame->loader().provisionalDocumentLoader(); Redundant space present on RHS; ...
5 years, 6 months ago (2015-06-22 06:55:16 UTC) #11
haraken
LGTM, much nicer!
5 years, 6 months ago (2015-06-22 07:13:17 UTC) #12
sof
will wait on oilpan results.
5 years, 6 months ago (2015-06-22 07:45:23 UTC) #13
sof
The "FrameLoader always detaches its DocumentLoaders" only held 95% of the time, so some more ...
5 years, 6 months ago (2015-06-22 11:23:52 UTC) #14
haraken
On 2015/06/22 11:23:52, sof wrote: > The "FrameLoader always detaches its DocumentLoaders" only held 95% ...
5 years, 6 months ago (2015-06-22 11:51:31 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1194003004/190001
5 years, 6 months ago (2015-06-22 14:04:40 UTC) #17
commit-bot: I haz the power
Committed patchset #11 (id:190001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197576
5 years, 6 months ago (2015-06-22 14:08:09 UTC) #18
Stephen Chennney
A revert of this CL (patchset #11 id:190001) has been created in https://codereview.chromium.org/1200043002/ by schenney@chromium.org. ...
5 years, 6 months ago (2015-06-22 17:56:49 UTC) #19
sof
The initial landing attempt got caught up in some unrelated test instability; addressed by http://code.google.com/p/chromium/issues/detail?id=503583. ...
5 years, 6 months ago (2015-06-23 15:17:42 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1194003004/210001
5 years, 6 months ago (2015-06-23 15:18:08 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60320)
5 years, 6 months ago (2015-06-23 16:07:01 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1194003004/210001
5 years, 6 months ago (2015-06-23 16:20:38 UTC) #27
commit-bot: I haz the power
5 years, 6 months ago (2015-06-23 17:10:37 UTC) #28
Message was sent while issue was closed.
Committed patchset #12 (id:210001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197664

Powered by Google App Engine
This is Rietveld 408576698