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

Issue 561753002: Oilpan: fix PromiseTracker's handling of its persistent promise wrappers. (Closed)

Created:
6 years, 3 months ago by sof
Modified:
6 years, 3 months ago
CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Oilpan: fix PromiseTracker's handling of its persistent promise wrappers. The handling of the PromiseDataWrapper in r181662 wasn't complete, as it failed to keep an Oilpan-reachable persistent to the GCed wrapper object, risking premature finalization. For Oilpan also, this wrapper object should only keep its references weakly alive. To make the tracing of these relationships work out, PromiseTracker is no longer a part object, but heap allocated. R=haraken BUG=340522 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181845

Patch Set 1 #

Total comments: 4

Patch Set 2 : Tidying #

Patch Set 3 : Remove redundant leakPtr() use #

Patch Set 4 : Add FIXME on using DOMWrapperWorld to hold PromiseTrackerWrapper instances #

Unified diffs Side-by-side diffs Delta from patch set Stats (+74 lines, -51 lines) Patch
M Source/core/inspector/InspectorDebuggerAgent.h View 2 chunks +3 lines, -2 lines 0 comments Download
M Source/core/inspector/InspectorDebuggerAgent.cpp View 6 chunks +10 lines, -9 lines 0 comments Download
M Source/core/inspector/PromiseTracker.h View 2 chunks +11 lines, -5 lines 0 comments Download
M Source/core/inspector/PromiseTracker.cpp View 1 2 3 3 chunks +50 lines, -35 lines 0 comments Download

Messages

Total messages: 19 (7 generated)
sof
Please take a look. A crasher due to attempting to keep a WeakMember<> directly to ...
6 years, 3 months ago (2014-09-10 13:48:09 UTC) #2
aandrey
FYI https://codereview.chromium.org/561753002/diff/1/Source/core/inspector/PromiseTracker.cpp File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/561753002/diff/1/Source/core/inspector/PromiseTracker.cpp#newcode112 Source/core/inspector/PromiseTracker.cpp:112: if (!vector->size()) if (vector->isEmpty()) https://codereview.chromium.org/561753002/diff/1/Source/core/inspector/PromiseTracker.cpp#newcode125 Source/core/inspector/PromiseTracker.cpp:125: PromiseDataWrapper(WeakPtrWillBeRawPtr<PromiseTracker::PromiseData> data, ...
6 years, 3 months ago (2014-09-10 14:42:18 UTC) #4
haraken
Hmm, the wrapper handling looks quite complicated. Can we simplify the wrapper handling by using ...
6 years, 3 months ago (2014-09-10 15:54:27 UTC) #5
sof
https://codereview.chromium.org/561753002/diff/1/Source/core/inspector/PromiseTracker.cpp File Source/core/inspector/PromiseTracker.cpp (right): https://codereview.chromium.org/561753002/diff/1/Source/core/inspector/PromiseTracker.cpp#newcode112 Source/core/inspector/PromiseTracker.cpp:112: if (!vector->size()) On 2014/09/10 14:42:18, aandrey wrote: > if ...
6 years, 3 months ago (2014-09-11 05:20:53 UTC) #6
sof
On 2014/09/10 15:54:27, haraken wrote: > Hmm, the wrapper handling looks quite complicated. > > ...
6 years, 3 months ago (2014-09-11 06:56:28 UTC) #7
haraken
On 2014/09/11 06:56:28, sof wrote: > On 2014/09/10 15:54:27, haraken wrote: > > Hmm, the ...
6 years, 3 months ago (2014-09-11 13:46:57 UTC) #8
sof
On 2014/09/11 13:46:57, haraken wrote: > On 2014/09/11 06:56:28, sof wrote: > > On 2014/09/10 ...
6 years, 3 months ago (2014-09-11 14:42:22 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/561753002/60001
6 years, 3 months ago (2014-09-11 14:42:54 UTC) #11
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/26635)
6 years, 3 months ago (2014-09-11 16:12:44 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/561753002/60001
6 years, 3 months ago (2014-09-11 17:02:26 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/561753002/60001
6 years, 3 months ago (2014-09-11 18:18:45 UTC) #18
commit-bot: I haz the power
6 years, 3 months ago (2014-09-11 19:56:07 UTC) #19
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 181845

Powered by Google App Engine
This is Rietveld 408576698