Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1641853003:
Create MediaValuesCached and TokenPreloadScanner on the parser thread (Closed)
|
Created:
4 years, 10 months ago by hiroshige Modified:
4 years, 10 months ago CC:
chromium-reviews, blink-reviews-css, blink-reviews-html_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, kinuko+watch, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@TRV_MediaValuesCached Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate MediaValuesCached and TokenPreloadScanner on the parser thread
Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main
thread, passed to and intended to be destructed on the background parser thread.
However, since https://codereview.chromium.org/1611303006, MediaValuesCached
is an Oilpan object and started to be destructed on the main thread, which
causes race conditions on the refcount of StringImpl in MediaValuesCachedData.
This CL removes the race condition by creating, using and destructing
TokenPreloadScanner and MediaValuesCached on the background parser thread.
Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData
(non-Oilpan object) is created on the main thread and passed to the background
parser thread.
This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it
is no longer safe to be passed across threads,
and defines CrossThreadCopier for MediaValuesCachedData instead.
BUG=581565, 478194, 585026
Committed: https://crrev.com/1e224f69bc03710f9f7c8c8f8d8f3526d004ea00
Cr-Commit-Position: refs/heads/master@{#374027}
Committed: https://crrev.com/b663c523bddc75b4b2009f8804b054e777e84a37
Cr-Commit-Position: refs/heads/master@{#374850}
Patch Set 1 #Patch Set 2 : #
Total comments: 9
Patch Set 3 : Rebase. #Patch Set 4 : Add explicit. #Patch Set 5 : #Patch Set 6 : Disable test on asan bots #Depends on Patchset: Messages
Total messages: 49 (22 generated)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641853003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641853003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641853003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641853003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread BUG= ========== to ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread BUG= ==========
hiroshige@chromium.org changed reviewers: + haraken@chromium.org, kouhei@chromium.org, yoav@yoav.ws
https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/MediaValuesCached.cpp (right): https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/MediaValuesCached.cpp:19: // FIXME - Added an assert here so we can better understand when a frame is present without its view(). I think we can now safely conclude that this doesn't really happen, so the frame->view() condition check can be eliminated. Maybe in a different CL though https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/MediaValuesCached.h (right): https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/MediaValuesCached.h:114: template<> struct CrossThreadCopierBase<false, false, false, MediaValuesCached::MediaValuesCachedData> { Where is this used? Is it implicitly called somewhere? https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:832: CachedDocumentParameters::create(document()), Isn't that racy? IIUC (and it's fairly possible I might not be...) CachedDocumentParameters::create(document()) and MediaValuesCachedData(*document()) are run in the context of the background thread, which means that we're passing document() over. At least in theory, I don't think that's safe.
Thanks for comments! https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/MediaValuesCached.h (right): https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/MediaValuesCached.h:114: template<> struct CrossThreadCopierBase<false, false, false, MediaValuesCached::MediaValuesCachedData> { On 2016/02/01 09:29:30, Yoav Weiss wrote: > Where is this used? Is it implicitly called somewhere? CrossThreadCopier is called inside threadSafeBind() in HTMLDocumentParser.cpp (around Line 827). https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:832: CachedDocumentParameters::create(document()), On 2016/02/01 09:29:30, Yoav Weiss wrote: > Isn't that racy? IIUC (and it's fairly possible I might not be...) > CachedDocumentParameters::create(document()) and > MediaValuesCachedData(*document()) are run in the context of the background > thread, which means that we're passing document() over. > > At least in theory, I don't think that's safe. I think: 1. HTMLDocumentParser::startBackgroundParser() is called on the main thread. 2. CachedDocumentParameters::create(document()) and MediaValuesCachedData(*document()) are also executed on the main thread. 3. Results of Step 2 is processed by CrossThreadCopier, and passed across threads. 4. BackgroundHTMLParser::start() is executed on the background parser thread. Because Step 2. is executed on the main thread, I expect this is safe.
Thanks for comments!
On 2016/02/01 09:42:11, hiroshige wrote: > Thanks for comments! > > https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/MediaValuesCached.h (right): > > https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/MediaValuesCached.h:114: template<> struct > CrossThreadCopierBase<false, false, false, > MediaValuesCached::MediaValuesCachedData> { > On 2016/02/01 09:29:30, Yoav Weiss wrote: > > Where is this used? Is it implicitly called somewhere? > > CrossThreadCopier is called inside threadSafeBind() in HTMLDocumentParser.cpp > (around Line 827). > > https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): > > https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:832: > CachedDocumentParameters::create(document()), > On 2016/02/01 09:29:30, Yoav Weiss wrote: > > Isn't that racy? IIUC (and it's fairly possible I might not be...) > > CachedDocumentParameters::create(document()) and > > MediaValuesCachedData(*document()) are run in the context of the background > > thread, which means that we're passing document() over. > > > > At least in theory, I don't think that's safe. > > I think: > 1. HTMLDocumentParser::startBackgroundParser() is called on the main thread. > 2. CachedDocumentParameters::create(document()) and > MediaValuesCachedData(*document()) are also executed on the main thread. > 3. Results of Step 2 is processed by CrossThreadCopier, and passed across > threads. > 4. BackgroundHTMLParser::start() is executed on the background parser thread. > > Because Step 2. is executed on the main thread, I expect this is safe. OK, that makes sense! :) LGTM!
LGTM https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/MediaValuesCached.cpp (right): https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/MediaValuesCached.cpp:19: // FIXME - Added an assert here so we can better understand when a frame is present without its view(). FIXME => TODO(hiroshige) https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h (right): https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h:63: CachedDocumentParameters(Document*); Add explicit.
lgtm
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641853003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641853003/60001
Description was changed from ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread BUG= ========== to ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main thread, passed to and intended to be destructed on the background parser thread. However, since https://codereview.chromium.org/1611303006, MediaValuesCached is an Oilpan object and started to be destructed on the main thread, which causes race conditions on the refcount of StringImpl in MediaValuesCachedData. This CL removes the race condition by creating, using and destructing TokenPreloadScanner and MediaValuesCached on the background parser thread. Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData (non-Oilpan object) is created on the main thread and passed to the background parser thread. This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it is no longer safe to be passed across threads, and defines CrossThreadCopier for MediaValuesCachedData instead. BUG=581565, 478194 ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641853003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641853003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/MediaValuesCached.cpp (right): https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/MediaValuesCached.cpp:19: // FIXME - Added an assert here so we can better understand when a frame is present without its view(). On 2016/02/01 09:29:30, Yoav Weiss wrote: > I think we can now safely conclude that this doesn't really happen, so the > frame->view() condition check can be eliminated. Maybe in a different CL though Done. I'll create another CL. Is this because Issues 367210 and 371084 are fixed? https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h (right): https://codereview.chromium.org/1641853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h:63: CachedDocumentParameters(Document*); On 2016/02/01 11:09:16, haraken wrote: > > Add explicit. Done.
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yoav@yoav.ws, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/1641853003/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641853003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641853003/80001
Message was sent while issue was closed.
Description was changed from ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main thread, passed to and intended to be destructed on the background parser thread. However, since https://codereview.chromium.org/1611303006, MediaValuesCached is an Oilpan object and started to be destructed on the main thread, which causes race conditions on the refcount of StringImpl in MediaValuesCachedData. This CL removes the race condition by creating, using and destructing TokenPreloadScanner and MediaValuesCached on the background parser thread. Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData (non-Oilpan object) is created on the main thread and passed to the background parser thread. This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it is no longer safe to be passed across threads, and defines CrossThreadCopier for MediaValuesCachedData instead. BUG=581565, 478194 ========== to ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main thread, passed to and intended to be destructed on the background parser thread. However, since https://codereview.chromium.org/1611303006, MediaValuesCached is an Oilpan object and started to be destructed on the main thread, which causes race conditions on the refcount of StringImpl in MediaValuesCachedData. This CL removes the race condition by creating, using and destructing TokenPreloadScanner and MediaValuesCached on the background parser thread. Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData (non-Oilpan object) is created on the main thread and passed to the background parser thread. This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it is no longer safe to be passed across threads, and defines CrossThreadCopier for MediaValuesCachedData instead. BUG=581565, 478194 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main thread, passed to and intended to be destructed on the background parser thread. However, since https://codereview.chromium.org/1611303006, MediaValuesCached is an Oilpan object and started to be destructed on the main thread, which causes race conditions on the refcount of StringImpl in MediaValuesCachedData. This CL removes the race condition by creating, using and destructing TokenPreloadScanner and MediaValuesCached on the background parser thread. Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData (non-Oilpan object) is created on the main thread and passed to the background parser thread. This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it is no longer safe to be passed across threads, and defines CrossThreadCopier for MediaValuesCachedData instead. BUG=581565, 478194 ========== to ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main thread, passed to and intended to be destructed on the background parser thread. However, since https://codereview.chromium.org/1611303006, MediaValuesCached is an Oilpan object and started to be destructed on the main thread, which causes race conditions on the refcount of StringImpl in MediaValuesCachedData. This CL removes the race condition by creating, using and destructing TokenPreloadScanner and MediaValuesCached on the background parser thread. Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData (non-Oilpan object) is created on the main thread and passed to the background parser thread. This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it is no longer safe to be passed across threads, and defines CrossThreadCopier for MediaValuesCachedData instead. BUG=581565, 478194 Committed: https://crrev.com/1e224f69bc03710f9f7c8c8f8d8f3526d004ea00 Cr-Commit-Position: refs/heads/master@{#374027} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1e224f69bc03710f9f7c8c8f8d8f3526d004ea00 Cr-Commit-Position: refs/heads/master@{#374027}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1679703002/ by thakis@chromium.org. The reason for reverting is: Somewhat speculative; https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20T... used to be green all the time, but it started being red 70% of the time recently, with SingleProcessTracingBrowserTest.TestMemoryInfra failing. Example: https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20T... .
Message was sent while issue was closed.
On 2016/02/07 18:20:25, Nico wrote: > A revert of this CL (patchset #5 id:80001) has been created in > https://codereview.chromium.org/1679703002/ by mailto:thakis@chromium.org. > > The reason for reverting is: Somewhat speculative; > https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20T... > used to be green all the time, but it started being red 70% of the time > recently, with SingleProcessTracingBrowserTest.TestMemoryInfra failing. Example: > https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20T... > . Unrelated; that unit test is LSan flaky for other reasons (not cleanly closing some tabs, not giving Blink a chance to prepare for LSan's detection pass). We should file a separate bug for the memory-infra folks to have a look. When I looked for it flaking last week, it didn't show..but looks like it is back.
Message was sent while issue was closed.
On 2016/02/07 18:59:45, sof wrote: > On 2016/02/07 18:20:25, Nico wrote: > > A revert of this CL (patchset #5 id:80001) has been created in > > https://codereview.chromium.org/1679703002/ by mailto:thakis@chromium.org. > > > > The reason for reverting is: Somewhat speculative; > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20T... > > used to be green all the time, but it started being red 70% of the time > > recently, with SingleProcessTracingBrowserTest.TestMemoryInfra failing. > Example: > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20T... > > . > > Unrelated; that unit test is LSan flaky for other reasons (not cleanly closing > some tabs, not giving Blink a chance to prepare for LSan's detection pass). We > should file a separate bug for the memory-infra folks to have a look. > > When I looked for it flaking last week, it didn't show..but looks like it is > back. Thanks for information! Filed https://crbug.com/585026. Because this CL fixes a thread-safety issue, I'd like to land this CL by M-50 branch cut.
Message was sent while issue was closed.
On 2016/02/08 06:56:58, hiroshige wrote: > On 2016/02/07 18:59:45, sof wrote: > > On 2016/02/07 18:20:25, Nico wrote: > > > A revert of this CL (patchset #5 id:80001) has been created in > > > https://codereview.chromium.org/1679703002/ by mailto:thakis@chromium.org. > > > > > > The reason for reverting is: Somewhat speculative; > > > > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20T... > > > used to be green all the time, but it started being red 70% of the time > > > recently, with SingleProcessTracingBrowserTest.TestMemoryInfra failing. > > Example: > > > > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20T... > > > . > > > > Unrelated; that unit test is LSan flaky for other reasons (not cleanly closing > > some tabs, not giving Blink a chance to prepare for LSan's detection pass). We > > should file a separate bug for the memory-infra folks to have a look. > > > > When I looked for it flaking last week, it didn't show..but looks like it is > > back. > > Thanks for information! > Filed https://crbug.com/585026. > > Because this CL fixes a thread-safety issue, I'd like to land this CL by M-50 > branch cut. +1 to relanding this before branching
Message was sent while issue was closed.
Description was changed from ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main thread, passed to and intended to be destructed on the background parser thread. However, since https://codereview.chromium.org/1611303006, MediaValuesCached is an Oilpan object and started to be destructed on the main thread, which causes race conditions on the refcount of StringImpl in MediaValuesCachedData. This CL removes the race condition by creating, using and destructing TokenPreloadScanner and MediaValuesCached on the background parser thread. Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData (non-Oilpan object) is created on the main thread and passed to the background parser thread. This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it is no longer safe to be passed across threads, and defines CrossThreadCopier for MediaValuesCachedData instead. BUG=581565, 478194 Committed: https://crrev.com/1e224f69bc03710f9f7c8c8f8d8f3526d004ea00 Cr-Commit-Position: refs/heads/master@{#374027} ========== to ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main thread, passed to and intended to be destructed on the background parser thread. However, since https://codereview.chromium.org/1611303006, MediaValuesCached is an Oilpan object and started to be destructed on the main thread, which causes race conditions on the refcount of StringImpl in MediaValuesCachedData. This CL removes the race condition by creating, using and destructing TokenPreloadScanner and MediaValuesCached on the background parser thread. Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData (non-Oilpan object) is created on the main thread and passed to the background parser thread. This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it is no longer safe to be passed across threads, and defines CrossThreadCopier for MediaValuesCachedData instead. BUG=581565, 478194 Committed: https://crrev.com/1e224f69bc03710f9f7c8c8f8d8f3526d004ea00 Cr-Commit-Position: refs/heads/master@{#374027} ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Description was changed from ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main thread, passed to and intended to be destructed on the background parser thread. However, since https://codereview.chromium.org/1611303006, MediaValuesCached is an Oilpan object and started to be destructed on the main thread, which causes race conditions on the refcount of StringImpl in MediaValuesCachedData. This CL removes the race condition by creating, using and destructing TokenPreloadScanner and MediaValuesCached on the background parser thread. Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData (non-Oilpan object) is created on the main thread and passed to the background parser thread. This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it is no longer safe to be passed across threads, and defines CrossThreadCopier for MediaValuesCachedData instead. BUG=581565, 478194 Committed: https://crrev.com/1e224f69bc03710f9f7c8c8f8d8f3526d004ea00 Cr-Commit-Position: refs/heads/master@{#374027} ========== to ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main thread, passed to and intended to be destructed on the background parser thread. However, since https://codereview.chromium.org/1611303006, MediaValuesCached is an Oilpan object and started to be destructed on the main thread, which causes race conditions on the refcount of StringImpl in MediaValuesCachedData. This CL removes the race condition by creating, using and destructing TokenPreloadScanner and MediaValuesCached on the background parser thread. Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData (non-Oilpan object) is created on the main thread and passed to the background parser thread. This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it is no longer safe to be passed across threads, and defines CrossThreadCopier for MediaValuesCachedData instead. BUG=581565, 478194, 585026 Committed: https://crrev.com/1e224f69bc03710f9f7c8c8f8d8f3526d004ea00 Cr-Commit-Position: refs/heads/master@{#374027} ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641853003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641853003/100001
I'll disable the test on asan bots (https://codereview.chromium.org/1680923003/) and reland this CL.
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 hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yoav@yoav.ws, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/1641853003/#ps100001 (title: "Disable test on asan bots")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1641853003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1641853003/100001
Message was sent while issue was closed.
Description was changed from ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main thread, passed to and intended to be destructed on the background parser thread. However, since https://codereview.chromium.org/1611303006, MediaValuesCached is an Oilpan object and started to be destructed on the main thread, which causes race conditions on the refcount of StringImpl in MediaValuesCachedData. This CL removes the race condition by creating, using and destructing TokenPreloadScanner and MediaValuesCached on the background parser thread. Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData (non-Oilpan object) is created on the main thread and passed to the background parser thread. This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it is no longer safe to be passed across threads, and defines CrossThreadCopier for MediaValuesCachedData instead. BUG=581565, 478194, 585026 Committed: https://crrev.com/1e224f69bc03710f9f7c8c8f8d8f3526d004ea00 Cr-Commit-Position: refs/heads/master@{#374027} ========== to ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main thread, passed to and intended to be destructed on the background parser thread. However, since https://codereview.chromium.org/1611303006, MediaValuesCached is an Oilpan object and started to be destructed on the main thread, which causes race conditions on the refcount of StringImpl in MediaValuesCachedData. This CL removes the race condition by creating, using and destructing TokenPreloadScanner and MediaValuesCached on the background parser thread. Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData (non-Oilpan object) is created on the main thread and passed to the background parser thread. This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it is no longer safe to be passed across threads, and defines CrossThreadCopier for MediaValuesCachedData instead. BUG=581565, 478194, 585026 Committed: https://crrev.com/1e224f69bc03710f9f7c8c8f8d8f3526d004ea00 Cr-Commit-Position: refs/heads/master@{#374027} ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main thread, passed to and intended to be destructed on the background parser thread. However, since https://codereview.chromium.org/1611303006, MediaValuesCached is an Oilpan object and started to be destructed on the main thread, which causes race conditions on the refcount of StringImpl in MediaValuesCachedData. This CL removes the race condition by creating, using and destructing TokenPreloadScanner and MediaValuesCached on the background parser thread. Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData (non-Oilpan object) is created on the main thread and passed to the background parser thread. This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it is no longer safe to be passed across threads, and defines CrossThreadCopier for MediaValuesCachedData instead. BUG=581565, 478194, 585026 Committed: https://crrev.com/1e224f69bc03710f9f7c8c8f8d8f3526d004ea00 Cr-Commit-Position: refs/heads/master@{#374027} ========== to ========== Create MediaValuesCached and TokenPreloadScanner on the parser thread Previously, TokenPreloadScanner and MediaValuesCached were allocated on the main thread, passed to and intended to be destructed on the background parser thread. However, since https://codereview.chromium.org/1611303006, MediaValuesCached is an Oilpan object and started to be destructed on the main thread, which causes race conditions on the refcount of StringImpl in MediaValuesCachedData. This CL removes the race condition by creating, using and destructing TokenPreloadScanner and MediaValuesCached on the background parser thread. Instead of MediaValuesCached (an Oilpan object), MediaValuesCachedData (non-Oilpan object) is created on the main thread and passed to the background parser thread. This CL removes MediaValuesCached::isSafeToSendToAnotherThread() because it is no longer safe to be passed across threads, and defines CrossThreadCopier for MediaValuesCachedData instead. BUG=581565, 478194, 585026 Committed: https://crrev.com/1e224f69bc03710f9f7c8c8f8d8f3526d004ea00 Cr-Commit-Position: refs/heads/master@{#374027} Committed: https://crrev.com/b663c523bddc75b4b2009f8804b054e777e84a37 Cr-Commit-Position: refs/heads/master@{#374850} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b663c523bddc75b4b2009f8804b054e777e84a37 Cr-Commit-Position: refs/heads/master@{#374850} |
