Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2743663003:
Un-DCHECK-guard ThreadRestrictionVerifier for strings. (Closed)
|
Created:
3 years, 9 months ago by meade_UTC10 Modified:
3 years, 9 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-wtf_chromium.org, chromium-reviews, dglazkov+blink, Mikhail, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUn-DCHECK-guard ThreadRestrictionVerifier for strings.
This should help us debug various String crashes we've been seeing
through the crash server, particularly the linked bug.
Note that this will definitely make strings slow on Canary, and will
possibly cause crashes other than the particular one I was looking
for. These crashes will all be security bugs though, so we should
get some valuable information regardless.
PSA to blink-dev that I am doing this investigation is here: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/SupcNcaTh_g
This CL should be reverted by 17 March 2017.
This CL should be reverted by 17 March 2017.
This CL should be reverted by 17 March 2017.
This CL should be reverted by 17 March 2017.
BUG=694520
Review-Url: https://codereview.chromium.org/2743663003
Cr-Commit-Position: refs/heads/master@{#456323}
Committed: https://chromium.googlesource.com/chromium/src/+/3a001533b29c0fe28f2354285b2c2ac8d57d8ce6
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase #
Messages
Total messages: 24 (14 generated)
Description was changed from ========== Un-DCHECK-guard ThreadRestrictionVerifier for strings. This should help us debug various String crashes we've been seeing through the crash server, particularly the linked bug. This CL should be reverted by 17 March 2017. Note that this will definitely make strings slow on Canary, and will possibly cause crashes other than the particular one I was looking for. These crashes will all be security bugs though, so we should get some valuable information regardless. BUG=694520 ========== to ========== Un-DCHECK-guard ThreadRestrictionVerifier for strings. This should help us debug various String crashes we've been seeing through the crash server, particularly the linked bug. Note that this will definitely make strings slow on Canary, and will possibly cause crashes other than the particular one I was looking for. These crashes will all be security bugs though, so we should get some valuable information regardless. PSA to blink-dev that I am doing this investigation is here: TBD This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. BUG=694520 ==========
meade@chromium.org changed reviewers: + esprehn@chromium.org, sigbjorn@opera.com
I will send a PSA to blink-dev, and attach it to the CL description before landing. Definitely won't land this until Monday (Sydney time) anyway.
esprehn@chromium.org changed reviewers: + csharrison@chromium.org, wez@chromium.org
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/2743663003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/RuleSet.cpp (right): https://codereview.chromium.org/2743663003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/RuleSet.cpp:353: // TODO(meade): crbug.com/694520 This is r455697, so will be rebased away before landing? https://codereview.chromium.org/2743663003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/StringImpl.h (right): https://codereview.chromium.org/2743663003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/StringImpl.h:40: // #if DCHECK_IS_ON() Just a suggestion if this is something we do once in a while -- add #define CHECK_STRING_THREAD_SAFETY(X) CHECK(X) // DCHECK() once reverted back to previous. or something similarly named. With an empty alternative if the check is not enabled & use it throughout.
Description was changed from ========== Un-DCHECK-guard ThreadRestrictionVerifier for strings. This should help us debug various String crashes we've been seeing through the crash server, particularly the linked bug. Note that this will definitely make strings slow on Canary, and will possibly cause crashes other than the particular one I was looking for. These crashes will all be security bugs though, so we should get some valuable information regardless. PSA to blink-dev that I am doing this investigation is here: TBD This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. BUG=694520 ========== to ========== Un-DCHECK-guard ThreadRestrictionVerifier for strings. This should help us debug various String crashes we've been seeing through the crash server, particularly the linked bug. Note that this will definitely make strings slow on Canary, and will possibly cause crashes other than the particular one I was looking for. These crashes will all be security bugs though, so we should get some valuable information regardless. PSA to blink-dev that I am doing this investigation is here: TBD This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. BUG=694520 ==========
sigbjornf@opera.com changed reviewers: - sigbjorn@opera.com
I'm excited for this change, thanks for moving forward on it. In your PSA could you include crbug.com/545926, which is one place we know does unsafe access and will cause crashes.
On 2017/03/10 13:13:34, Charlie Harrison wrote: > I'm excited for this change, thanks for moving forward on it. > > In your PSA could you include crbug.com/545926, which is one place we know does > unsafe access and will cause crashes. Will do, thank you!
https://codereview.chromium.org/2743663003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/RuleSet.cpp (right): https://codereview.chromium.org/2743663003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/RuleSet.cpp:353: // TODO(meade): crbug.com/694520 On 2017/03/10 07:53:33, sof wrote: > This is r455697, so will be rebased away before landing? Oops, yes, done. https://codereview.chromium.org/2743663003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/StringImpl.h (right): https://codereview.chromium.org/2743663003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/StringImpl.h:40: // #if DCHECK_IS_ON() On 2017/03/10 07:53:33, sof wrote: > Just a suggestion if this is something we do once in a while -- add > > #define CHECK_STRING_THREAD_SAFETY(X) CHECK(X) // DCHECK() once reverted back > to previous. > > or something similarly named. With an empty alternative if the check is not > enabled & use it throughout. I'm not sure if it will be - I'll consider doing that if we find lots of things when this gets landed.
lgtm
The CQ bit was checked by esprehn@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...
Description was changed from ========== Un-DCHECK-guard ThreadRestrictionVerifier for strings. This should help us debug various String crashes we've been seeing through the crash server, particularly the linked bug. Note that this will definitely make strings slow on Canary, and will possibly cause crashes other than the particular one I was looking for. These crashes will all be security bugs though, so we should get some valuable information regardless. PSA to blink-dev that I am doing this investigation is here: TBD This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. BUG=694520 ========== to ========== Un-DCHECK-guard ThreadRestrictionVerifier for strings. This should help us debug various String crashes we've been seeing through the crash server, particularly the linked bug. Note that this will definitely make strings slow on Canary, and will possibly cause crashes other than the particular one I was looking for. These crashes will all be security bugs though, so we should get some valuable information regardless. PSA to blink-dev that I am doing this investigation is here: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/SupcNcaTh_g This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. BUG=694520 ==========
The CQ bit was unchecked by meade@chromium.org
The CQ bit was checked by meade@chromium.org
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": 20001, "attempt_start_ts": 1489375273271590,
"parent_rev": "f6c436401f1cb45525882a95950af68f6a1203ee", "commit_rev":
"3a001533b29c0fe28f2354285b2c2ac8d57d8ce6"}
Message was sent while issue was closed.
Description was changed from ========== Un-DCHECK-guard ThreadRestrictionVerifier for strings. This should help us debug various String crashes we've been seeing through the crash server, particularly the linked bug. Note that this will definitely make strings slow on Canary, and will possibly cause crashes other than the particular one I was looking for. These crashes will all be security bugs though, so we should get some valuable information regardless. PSA to blink-dev that I am doing this investigation is here: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/SupcNcaTh_g This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. BUG=694520 ========== to ========== Un-DCHECK-guard ThreadRestrictionVerifier for strings. This should help us debug various String crashes we've been seeing through the crash server, particularly the linked bug. Note that this will definitely make strings slow on Canary, and will possibly cause crashes other than the particular one I was looking for. These crashes will all be security bugs though, so we should get some valuable information regardless. PSA to blink-dev that I am doing this investigation is here: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/SupcNcaTh_g This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. This CL should be reverted by 17 March 2017. BUG=694520 Review-Url: https://codereview.chromium.org/2743663003 Cr-Commit-Position: refs/heads/master@{#456323} Committed: https://chromium.googlesource.com/chromium/src/+/3a001533b29c0fe28f2354285b2c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/3a001533b29c0fe28f2354285b2c...
Message was sent while issue was closed.
Patchset #3 (id:40001) has been deleted
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2753953002/ by meade@chromium.org. The reason for reverting is: Reverting as I finish up my investigation. BUG=701361. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
