stylo: we're doing Gecko URI stuff on background threads, which is bad

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
critical
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: bz, Assigned: cjku)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

A debug build fails the mainthread assert in URLValueData::GetURI.  An opt build crashes.  I haven't looked into where that happens.

Stack to the assert:

  * frame #0: 0x0000000106916b1a XUL`mozilla::css::URLValueData::GetURI(this=0x00000001256eb630) const + 106 at nsCSSValue.cpp:2853
    frame #1: 0x000000010691b6f2 XUL`mozilla::css::URLValueData::HasRef(this=0x00000001256eb630) const + 82 at nsCSSValue.cpp:2875
    frame #2: 0x00000001069c747e XUL`nsStyleImageLayers::Layer::CalcDifference(this=0x00000001256b0fc0, aNewLayer=0x0000000125645ee0) const + 142 at nsStyleStruct.cpp:3031
    frame #3: 0x00000001069c0d1a XUL`nsStyleImageLayers::CalcDifference(this=0x0000000125645eb0, aNewLayers=0x00000001256b0f90, aType=Background) const + 250 at nsStyleStruct.cpp:2661
    frame #4: 0x00000001069c9ac9 XUL`nsStyleBackground::CalcDifference(this=0x0000000125645eb0, aNewData=0x00000001256b0f90) const + 105 at nsStyleStruct.cpp:3112
    frame #5: 0x00000001069a74f0 XUL`nsChangeHint nsStyleContext::CalcStyleDifferenceInternal<FakeStyleContext>(this=0x0000000164538068, aNewContext=0x000070000c3cd268, aEqualStructs=0x000070000c3cd2ac, aSamePointerStructs=0x000070000c3cd2a8) + 5184 at nsStyleContext.cpp:1083
    frame #6: 0x00000001069a6075 XUL`nsStyleContext::CalcStyleDifference(this=0x0000000164538068, aNewComputedValues=0x000000012561a2c0, aEqualStructs=0x000070000c3cd2ac, aSamePointerStructs=0x000070000c3cd2a8) + 69 at nsStyleContext.cpp:1253
    frame #7: 0x0000000106841444 XUL`::Gecko_CalcStyleDifference(aOldStyleContext=0x0000000164538068, aComputedValues=0x000000012561a2c0) + 212 at ServoBindings.cpp:316
    frame #8: 0x000000010b050dca XUL`style::gecko::restyle_damage::{{impl}}::compute(source=0x0000000164538068, new_style=0x000070000c3cd5c8) + 90 at restyle_damage.rs:53

and then going up to style::parallel::traverse_nodes etc.

Presumably treeherder is not catching this because we don't do parallel traversal there and don't have the static analysis up and running yet...

I expect this is a regression from bug 1343964.
CalcStyleDifference should be using the DefinitelyEquals stuff.
Flags: needinfo?(xidorn+moz)
I don't expect this to be a regression from bug 1343964. The related code inside nsStyleImageLayers::Layer::CalcDifference is from bug 1345377 predates bug 1343964. If it didn't show up before, it may be that related values were not computed correctly before the URI fixes in bug 1343964 and its dependencies.

Having a look at the code, it is not about DefinitelyEquals. The code is definitely using DefeinitelyEquals, the issue is that we need to check whether the URI is for a fragment, which would needs an additional change hint [1], and the current URLValueData::HasRef() implementation relies on GetURI which is unsafe for other threads.

One solution in my mind is to make HasRef checks the string buffer directly if the URI is not resolved, but that could be error-prone, since we may end up implementing part of URI parsing.

I'm not sure what can we do with it. heycam?


[1] https://dxr.mozilla.org/mozilla-central/rev/35c7be9c2db288d1d449e3cc586c4164d642c5fd/layout/style/nsStyleStruct.cpp#2987-3013
Flags: needinfo?(xidorn+moz) → needinfo?(cam)
I wonder, if it would be correct to just scan the string and see if there is any hash character.
Priority: -- → P1
Blocks: 1243581
Since we only use this to determine whether to add UpdateOverflow to the returned hint, it's probably fine to use an approximation like "does this URL contain a hash character".

CJ, you're planning on changing this code (at least, to get rid of mSourceURI).  Will there be any need to call HasRef() in CalcDifference once you've made your changes?  (Which bug is that?)
Flags: needinfo?(cam) → needinfo?(cku)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

7 months ago
(In reply to Cameron McCormack (:heycam) from comment #4)
> Since we only use this to determine whether to add UpdateOverflow to the
> returned hint, it's probably fine to use an approximation like "does this
> URL contain a hash character".
> 
> CJ, you're planning on changing this code (at least, to get rid of
> mSourceURI).  Will there be any need to call HasRef() in CalcDifference once
> you've made your changes?  (Which bug is that?)

Bug 1352096. Unfortunately, nsStyeImage::mType can not be used to determine whether there is a fragment here.
Assignee: nobody → cku
Flags: needinfo?(cku)
(Assignee)

Updated

7 months ago
Attachment #8856418 - Flags: review?(cam)
Attachment #8856392 - Flags: review?(cam)

Updated

7 months ago
Blocks: 1318187

Updated

7 months ago
Duplicate of this bug: 1355253
Blocks: 1354215
Comment on attachment 8856418 [details]
Bug 1354772 - Part 1. Compute URLValueData::mIsLocalRef when need.

https://reviewboard.mozilla.org/r/128380/#review131224
Attachment #8856418 - Flags: review?(cam) → review+
Comment on attachment 8856392 [details]
Bug 1354772 - Part 2. Compute URLValueData::mMightHaveRef when need.

https://reviewboard.mozilla.org/r/128320/#review131226

::: layout/style/nsCSSValue.cpp:61
(Diff revision 2)
> +    if (*current == '/' || *current == '\\' || *current == ':') {
> +      return false;
> +    }

I don't think this is correct, since you can write:

  http://example.org/something#abc/def

and that is a valid URL where the fragment is "#abc/def".  So unfortunately I think we need to look right back until the start of the string.

Given that, should we just look from the start instead of the end?

::: layout/style/nsCSSValue.cpp:2899
(Diff revision 2)
>  
>    return mIsLocalRef.value();
>  }
>  
>  bool
>  css::URLValueData::HasRef() const

Now that this function is not returning a definite value, but an estimate, maybe we should rename it?  If we want to use "Definitely" in the name, then I guess we would need to invert the function, i.e. call it "DefinitelyHasNoRef".  Or, we can just rename this one to "MightHaveRef" (and make that global function ::MightHaveRef.)

Please then add a comment above its declaration in nsCSSValue.h saying that it takes a guess as to whether the URL has a fragment, by searching for a hash character, but that it definitely returns false if we know it can't have a fragment because it has no hash character.
Attachment #8856392 - Flags: review?(cam) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8856862 - Flags: review?(cam)
Comment on attachment 8856392 [details]
Bug 1354772 - Part 2. Compute URLValueData::mMightHaveRef when need.

https://reviewboard.mozilla.org/r/128320/#review131332
Attachment #8856392 - Flags: review?(cam) → review+
Comment on attachment 8856862 [details]
Bug 1354772 - Part 3. Correct the comment in Layer::CalcDifference.

https://reviewboard.mozilla.org/r/128776/#review131336
Attachment #8856862 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

6 months ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/244d25b4ee32
Part 1. Compute URLValueData::mIsLocalRef when need. r=heycam
https://hg.mozilla.org/integration/autoland/rev/2f467619e00c
Part 2. Compute URLValueData::mMightHaveRef when need. r=heycam
https://hg.mozilla.org/integration/autoland/rev/297193ba81a1
Part 3. Correct the comment in Layer::CalcDifference. r=heycam

Comment 20

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/244d25b4ee32
https://hg.mozilla.org/mozilla-central/rev/2f467619e00c
https://hg.mozilla.org/mozilla-central/rev/297193ba81a1
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.