Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2673163002:
[wrapper-tracing] Reset TLS on disposing (Closed)
|
Created:
3 years, 10 months ago by Michael Lippautz Modified:
3 years, 10 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[wrapper-tracing] Reset TLS on disposing
DOMWrapperWorlds can manually be disposed, so we also need to reset the
TLS for worker threads in there.
BUG=chromium:688120
Review-Url: https://codereview.chromium.org/2673163002
Cr-Commit-Position: refs/heads/master@{#448445}
Committed: https://chromium.googlesource.com/chromium/src/+/b0a29222e2a01324bce86c9a1880678cedcb4db1
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address comment #
Total comments: 2
Patch Set 3 : Address one more comment #Messages
Total messages: 32 (19 generated)
Description was changed from ========== [wrapper-tracing] Reset TLS on disposing DOMWrapperWorlds can manually be disposed, so we also need to reset the TLS for worker threads in there. BUG=chromium:688120 ========== to ========== [wrapper-tracing] Reset TLS on disposing DOMWrapperWorlds can manually be disposed, so we also need to reset the TLS for worker threads in there. BUG=chromium:688120 ==========
mlippautz@chromium.org changed reviewers: + haraken@chromium.org, jochen@chromium.org
PTAL; This one should also be back merged I guess I have trouble writing a proper test as we would basically need to create a worker, manually dispose it (by leaving a scope) but still trigger an incremental marking step/finalization after that.
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. This makes sense. Also I'd be happy if you could consider addressing https://codereview.chromium.org/2665733002/diff/140001/third_party/WebKit/Sou... when you have a chance.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
lgtm https://codereview.chromium.org/2673163002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/2673163002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:208: if (m_worldId == WorkerWorldId) { isWorkerWorld() & no braces needed.
is it possible to write a test for this?
https://codereview.chromium.org/2673163002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/2673163002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:208: if (m_worldId == WorkerWorldId) { On 2017/02/04 13:01:22, sof wrote: > isWorkerWorld() & no braces needed. Done.
On 2017/02/04 09:31:08, haraken wrote: > LGTM. This makes sense. > > Also I'd be happy if you could consider addressing > https://codereview.chromium.org/2665733002/diff/140001/third_party/WebKit/Sou... > when you have a chance. Yep, maybe next week.
On 2017/02/04 18:13:15, jochen (travelling til Feb 4) wrote: > is it possible to write a test for this? I tried to find the right entry points but failed, that's why I was asking for input in my initial message. We'd need to create a worker thread attached to a given isolate that is then destroyed only before triggering a GC.
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/04 at 18:22:28, mlippautz wrote: > On 2017/02/04 18:13:15, jochen (travelling til Feb 4) wrote: > > is it possible to write a test for this? > > I tried to find the right entry points but failed, that's why I was asking for input in my initial message. > > We'd need to create a worker thread attached to a given isolate that is then destroyed only before triggering a GC. what about adding a test here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/workers/W... where you create a worker, start the shutdown sequence, and then manually invoke blink::ScriptWrappableVisitor::markWrappersInAllWorlds for some scriptwrapable?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2673163002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/2673163002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:144: if (workerWorld()) { How about saving this away in a local so as to avoid doing two TLS lookups on this code path?
On 2017/02/04 18:37:57, jochen wrote: > On 2017/02/04 at 18:22:28, mlippautz wrote: > > On 2017/02/04 18:13:15, jochen (travelling til Feb 4) wrote: > > > is it possible to write a test for this? > > > > I tried to find the right entry points but failed, that's why I was asking for > input in my initial message. > > > > We'd need to create a worker thread attached to a given isolate that is then > destroyed only before triggering a GC. > > what about adding a test here: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/workers/W... > where you create a worker, start the shutdown sequence, and then manually invoke > blink::ScriptWrappableVisitor::markWrappersInAllWorlds for some scriptwrapable? Tried now several combinations in ThreadedWorkletTest.cpp. I was able to trigger the case (by just disposing the WorkerOrWorkletScriptController but unable to properly shutdown then, i.e., some other component crashed on me then. Since this might be important for stability, I am landing now. If you have any further suggestions I am happy to follow up with a test.
https://codereview.chromium.org/2673163002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp (right): https://codereview.chromium.org/2673163002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DOMWrapperWorld.cpp:144: if (workerWorld()) { On 2017/02/06 12:48:59, sof wrote: > How about saving this away in a local so as to avoid doing two TLS lookups on > this code path? Done.
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mlippautz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/2673163002/#ps40001 (title: "Address one more comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486423968038470,
"parent_rev": "52a008bec9f907b1b142486d2f3feb036de02c55", "commit_rev":
"b0a29222e2a01324bce86c9a1880678cedcb4db1"}
Message was sent while issue was closed.
Description was changed from ========== [wrapper-tracing] Reset TLS on disposing DOMWrapperWorlds can manually be disposed, so we also need to reset the TLS for worker threads in there. BUG=chromium:688120 ========== to ========== [wrapper-tracing] Reset TLS on disposing DOMWrapperWorlds can manually be disposed, so we also need to reset the TLS for worker threads in there. BUG=chromium:688120 Review-Url: https://codereview.chromium.org/2673163002 Cr-Commit-Position: refs/heads/master@{#448445} Committed: https://chromium.googlesource.com/chromium/src/+/b0a29222e2a01324bce86c9a1880... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b0a29222e2a01324bce86c9a1880... |
