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.

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

I wonder, if it would be correct to just scan the string and see if there is any hash character.

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?)

(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.

Comment on attachment 8856418 [details] Bug 1354772 - Part 1. Compute URLValueData::mIsLocalRef when need. https://reviewboard.mozilla.org/r/128380/#review131224

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.

Comment on attachment 8856392 [details] Bug 1354772 - Part 2. Compute URLValueData::mMightHaveRef when need. https://reviewboard.mozilla.org/r/128320/#review131332

Comment on attachment 8856862 [details] Bug 1354772 - Part 3. Correct the comment in Layer::CalcDifference. https://reviewboard.mozilla.org/r/128776/#review131336

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

https://hg.mozilla.org/mozilla-central/rev/244d25b4ee32 https://hg.mozilla.org/mozilla-central/rev/2f467619e00c https://hg.mozilla.org/mozilla-central/rev/297193ba81a1