New issue
Advanced search Search tips

Issue 689044 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Data race in blink::Document::baseURLForOverride

Project Member Reported by ClusterFuzz, Feb 6 2017

Issue description

Cc: msrchandra@chromium.org
Labels: Test-Predator-Correct-CLs M-58
Owner: csharrison@chromium.org
Status: Assigned (was: Untriaged)
Assigning to the concern owner from Predator results --
The result is a list of CLs that change the crashed files. 

Author: csharrison
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src/+/36ecc9ec6ab699d9300f50fd4ca26030acad3e26
Time: Thu Jan 05 00:52:37 2017
File StringImpl.cpp is changed in this cl (and is part of stack frame #0, "WTF::StringImpl::updateContainsOnlyASCII")
Minimum distance from crash line to modified line: 5. (file: StringImpl.cpp, crashed on: 319, modified: 314).

@csharrison -- Could you please look into the issue, kindly re-assign if this is not related to your changes.
Thank You.
Cc: esprehn@chromium.org
Components: Blink>Internals>WTF
Hm this seems like a real race, but the regression range doesn't seem to make too much sense.

My guess is that this is fallout from a combination of esprehns change [1] and the change listed in #1. Basically, my guess is we avoided the race before because we calculated updateContainsOnlyASCII() before the worker thread got to do it, via some code path that used to call equal() on the main thread that now calls stringViewEqual().

Probably the safest thing is to make sure these static local Strings have all their optimizations applied right away.

[1] https://codereview.chromium.org/2614123002/

Is the race safe since we're computing what is effectively an idempotent value? I don't know if this is undefined behavior because of the bitfields and how compilers do that stuff. I think we compute the hash of static strings up front though, so we should probably also compute the isascii bit... Actually can we just assert all static strings are ASCII and force the bit true?
I think technically per C++ semantics an "idempotent" race is still a race, but I have less experience with how much it matters in practice.

I like the idea to assert all static strings are ASCII and force the bit, but this one will be a bit uglier because it's just a static KURL and doesn't actually point to an underlying static string. Additionally, KURLs themselves are not immutable like StringImpl, so merely having blankURL accessed on multiple threads is buggy.

One solution would be to make a version of "static" KURLs that does something fancy, but it seems over-engineered. I would prefer to keep KURLs  thread unsafe and do something like thread safe static local, or remove accesses from other threads.

One more thing is that there were recently some bugs in Chrome that failed isAboutBlank checks when the URL looks like "about:blank?somequery". GURL was updated to be robust to these cases and maybe we should make KURL too.
Project Member

Comment 5 by ClusterFuzz, Mar 10 2017

Status: WontFix (was: Assigned)
ClusterFuzz testcase 5457617800658944 is flaky and no longer reproduces, so closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Status: Assigned (was: WontFix)
I don't think clusterfuzz is wrong per-se, but we should still probably fix this.
Cc: csharrison@chromium.org
Owner: ----
Status: Available (was: Assigned)
I don't have bandwidth for this ATM. I think we could probably have a version of static, const KURLs that are immutable.

I had a patch up here that I didn't finish:
https://codereview.chromium.org/2680933005/
Project Member

Comment 8 by sheriffbot@chromium.org, Jul 10

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: -msrchandra@chromium.org kkaluri@chromium.org
Labels: CF-NeedsTriage
Unable to find actual suspect through code search and also observing no CL's under regression range, hence adding appropriate label and requesting someone from dev team to look in to this issue.

Thanks!

Sign in to add a comment