Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1724263004:
V8 minor GC should not ignore hasPendingActivity (Closed)
|
Created:
4 years, 10 months ago by haraken Modified:
4 years, 9 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionV8 minor GC should not ignore hasPendingActivity
This is a potential fix for bug 586183.
It is wrong to return immediately when toScriptWrappable(wrapper)->hasPendingActivity returns true. It must call v8::Persistent::MarkActive() to keep alive the wrapper.
BUG=586183
Committed: https://crrev.com/097eced3f582b417c5857a25f645aa04c8174f67
Cr-Commit-Position: refs/heads/master@{#378277}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Messages
Total messages: 28 (12 generated)
Description was changed from ========== Remove unnecessary code from V8 minor GC's proglue BUG= ========== to ========== V8 minor GC should not ignore hasPendingActivity. This is a potential fix for bug 586183. It is wrong to return immediately when toScriptWrappable(wrapper)->hasPendingActivity returns true. It must call v8::Persistent::MarkActive() to keep alive the wrapper. However, in the first place, we won't need to check hasPendingActivity because hasPendingActivity for non-Nodes are already handled at line 118 and hasPendingActivity for Nodes are handled at line 136. So this CL simply removes the hasPendingActivity check (instead of adding MarkActive). BUG=586183 ==========
haraken@chromium.org changed reviewers: + jochen@chromium.org, kbr@chromium.org
PTAL
lgtm
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1724263004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1724263004/20001
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/1724263004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (left): https://codereview.chromium.org/1724263004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:125: const WrapperTypeInfo* type = toWrapperTypeInfo(wrapper); You still need this, I think, but need to add marking to it (like there used to be.) i.e., the hasPendingActivity() check below is only for image elements.
The CQ bit was unchecked by sigbjornf@opera.com
(unchecked commit box.)
https://codereview.chromium.org/1724263004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (left): https://codereview.chromium.org/1724263004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:125: const WrapperTypeInfo* type = toWrapperTypeInfo(wrapper); On 2016/02/25 13:08:13, sof wrote: > You still need this, I think, but need to add marking to it (like there used to > be.) i.e., the hasPendingActivity() check below is only for image elements. Ah, nice catch. HTMLMediaElement needs to be marked as well. I'll update the CL.
"proglue" would be an awesome alternative name for bindings/ btw :-)
Done. I'll remove code for hasPendingActivity of HTMLImageElement in a separate CL. Now that I moved hasPendingActivity to ScriptWrappable, we no longer need to special-case HTMLImageElement.
Description was changed from ========== V8 minor GC should not ignore hasPendingActivity. This is a potential fix for bug 586183. It is wrong to return immediately when toScriptWrappable(wrapper)->hasPendingActivity returns true. It must call v8::Persistent::MarkActive() to keep alive the wrapper. However, in the first place, we won't need to check hasPendingActivity because hasPendingActivity for non-Nodes are already handled at line 118 and hasPendingActivity for Nodes are handled at line 136. So this CL simply removes the hasPendingActivity check (instead of adding MarkActive). BUG=586183 ========== to ========== V8 minor GC should not ignore hasPendingActivity This is a potential fix for bug 586183. It is wrong to return immediately when toScriptWrappable(wrapper)->hasPendingActivity returns true. It must call v8::Persistent::MarkActive() to keep alive the wrapper. BUG=586183 ==========
On 2016/02/25 14:05:02, haraken wrote: > Done. > > I'll remove code for hasPendingActivity of HTMLImageElement in a separate CL. > Now that I moved hasPendingActivity to ScriptWrappable, we no longer need to > special-case HTMLImageElement. less magic, nice. lgtm.
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1724263004/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1724263004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1724263004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks haraken for tracking this down. Great. lgtm
Commented on http://crbug.com/586183 -- these tests are still flaky. I think the cleanup CL https://crrev.com/04f6d5b221c41d9110c36d5464f427162ddd1532 may have even increased the flakiness. This CL still seems to be needed for correctness. Can you please land this?
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1724263004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1724263004/40001
Message was sent while issue was closed.
Description was changed from ========== V8 minor GC should not ignore hasPendingActivity This is a potential fix for bug 586183. It is wrong to return immediately when toScriptWrappable(wrapper)->hasPendingActivity returns true. It must call v8::Persistent::MarkActive() to keep alive the wrapper. BUG=586183 ========== to ========== V8 minor GC should not ignore hasPendingActivity This is a potential fix for bug 586183. It is wrong to return immediately when toScriptWrappable(wrapper)->hasPendingActivity returns true. It must call v8::Persistent::MarkActive() to keep alive the wrapper. BUG=586183 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== V8 minor GC should not ignore hasPendingActivity This is a potential fix for bug 586183. It is wrong to return immediately when toScriptWrappable(wrapper)->hasPendingActivity returns true. It must call v8::Persistent::MarkActive() to keep alive the wrapper. BUG=586183 ========== to ========== V8 minor GC should not ignore hasPendingActivity This is a potential fix for bug 586183. It is wrong to return immediately when toScriptWrappable(wrapper)->hasPendingActivity returns true. It must call v8::Persistent::MarkActive() to keep alive the wrapper. BUG=586183 Committed: https://crrev.com/097eced3f582b417c5857a25f645aa04c8174f67 Cr-Commit-Position: refs/heads/master@{#378277} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/097eced3f582b417c5857a25f645aa04c8174f67 Cr-Commit-Position: refs/heads/master@{#378277} |
