As I was getting my almost complete background-image patches ready to submit today, I realised that our handling of nsIURI objects is more problematic than I first thought. To handle -moz-binding, which uses a URLValue object that has a lazily resolved nsIURI in it, we set the unresolved string and leave it until later on the main thread in the frame constructor (or whereever) to ask the URLValue to resolve and return the nsIURI. This works fine, but now that we call CalcStyleDifference off the main thread, we can end up calling nsIURI::Equals, which surely isn't thread safe if the nsIURI implementation is backed by JS. Obviously this problem applies to all other properties that store an nsIURI. I think nsIPrincipals, which also get compared in URLValueData::operator==, are also be a problem, since they can call into nsIURI methods. (And I'm not sure how thread safe the other things its Equals method does are.) We could replace all nsIURIs in style structs with objects that always keep around the relative URL string and the base URL as a string, for comparison purposes. For -moz-binding, which I think is the only one that cares about it, we would also keep around a serialization of the nsIPrincipal. (I assume such serializations are enough for equals comparisons?) This solution doesn't sound great, but I'm not sure what else we can do.

What if we stored the Rust URL? We might have to do it anyway, for the lazy conversion.

One issue we need to fix is that the specified Url values Servo generates during style sheet parsing are resolved to absolute URLs. That should be something done during computed value generation, though. I think the resolution to absolute URLs needs to be done by Gecko code, at least for now, since it will support all these different URL schemes such as jar:, chrome:, etc., and any registered by add-ons. So if we want to store the Rust Url object in the style struct for lazy resolution to an nsIURI, it probably doesn't have any advantage over grabbing a string out of it and storing the string. (At least if we convert the specified URL value to a string during computed value setting, we won't have to go back to Rust later from C++ to grab the string out.)

For the purpose of calling CalcDifference, I wonder if just comparing nsIPrincipal pointers it sufficient. For the common case of restyling and the -moz-binding was set from the same style sheet, we'll get the very same nsIPrincipal object off the sheet. If this is fine, then we don't have to worry about serializing the principal.

See also bug 1297851. IIUC, this could be solved by having URLValue store both base URI and string, as well as a lazily-resolved absolute URI (which would basically just be a cache for main thread consumers). Is that right? I think pointer-comparing principals and base URIs is almost certainly fine, unless it's ever a correctness problem to say things are not equal when they actually are (which would be exceedingly rare).

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4) > IIUC, this could be solved by having URLValue store both base URI and > string, as well as a lazily-resolved absolute URI (which would basically > just be a cache for main thread consumers). Is that right? Yes. -moz-binding uses URLValue for computed value storage, but for other image-typed properties we will either need to grab the base nsIURI and specified url() string off the URLValue and store them in the style struct, or just change to using URLValue for these properties too. The latter is a smaller change, so I'll try that. > I think pointer-comparing principals and base URIs is almost certainly fine, > unless it's ever a correctness problem to say things are not equal when they > actually are (which would be exceedingly rare). Yeah. I'll make it clear with a method name, e.g. MightNotBeEqual or something similar.

Comment on attachment 8785911 [details] Bug 1297963 - Part 1: Preserve base URI on URLValueData objects. https://reviewboard.mozilla.org/r/74922/#review72758 ::: dom/base/nsAttrValue.cpp:1737 (Diff revision 1) > } > #endif > > MiscContainer* cont = GetMiscContainer(); > mozilla::css::URLValue* url = cont->mValue.mURL; > mozilla::css::ImageValue* image = nit: probably should get rid of that whitespace while you're at it

Comment on attachment 8785911 [details] Bug 1297963 - Part 1: Preserve base URI on URLValueData objects. https://reviewboard.mozilla.org/r/74922/#review72868 r=me with the question answered. ::: layout/style/nsCSSValue.cpp:2643 (Diff revision 1) > nsCSSValue::GetBufferValue(aOther.mString)) == 0 && > (GetURI() == aOther.GetURI() || // handles null == null > (mURI && aOther.mURI && > NS_SUCCEEDED(mURI->Equals(aOther.mURI, &eq)) && > eq)) && > + (mBaseURI == aOther.mBaseURI || Do we really need to check the base uri here? Seems to me that if the uris and principals are equal we should be good to go?

Comment on attachment 8785912 [details] Bug 1297963 - Part 2: Add URLValueData comparison functions that work OMT. https://reviewboard.mozilla.org/r/74924/#review72870

Comment on attachment 8785913 [details] Bug 1297963 - Part 3: Use OMT-safe function for -moz-binding comparisons in CalcDifference. https://reviewboard.mozilla.org/r/74926/#review72872

Comment on attachment 8785914 [details] Bug 1297963 - Part 4: Remove unused URLValueData comparison functions. https://reviewboard.mozilla.org/r/74928/#review72874

Comment on attachment 8785911 [details] Bug 1297963 - Part 1: Preserve base URI on URLValueData objects. https://reviewboard.mozilla.org/r/74922/#review72868 > Do we really need to check the base uri here? > > Seems to me that if the uris and principals are equal we should be good to go? After these patches, URLValueData::operator== will only be used on objects representing specified values (by being called from nsCSSValue::operator==). It's pretty unclear to me what the semantics of nsCSSValue::operator== are, so it seemed best if we compare all the data members. For computed values, we'll use the specific named comparison functions like DefinitelyEqualURIs, so that it's clear what is being compared.

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e73da71408a3 Part 1: Preserve base URI on URLValueData objects. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/a2e337d5aa02 Part 2: Add URLValueData comparison functions that work OMT. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1ac59cfe8f Part 3: Use OMT-safe function for -moz-binding comparisons in CalcDifference. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/dedd56fa5c54 Part 4: Remove unused URLValueData comparison functions. r=emilio

Backed out for the same pile of browser-chrome failures that were in the last Try push posted to this bug... https://treeherder.mozilla.org/logviewer.html#?job_id=35108310&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=35108405&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=35108388&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=35108359&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=35107490&repo=mozilla-inbound etc...

And https://treeherder.mozilla.org/logviewer.html#?job_id=35107950&repo=mozilla-inbound

Sorry, I misjudged those failures as not being related to my patches. (The presence of "binding" in the name of one of those tests should've been a clue.)

Comment on attachment 8785912 [details] Bug 1297963 - Part 2: Add URLValueData comparison functions that work OMT. https://reviewboard.mozilla.org/r/74924/#review74368 ::: layout/style/nsCSSValue.cpp:2686 (Diff revision 1) > + NS_strcmp(nsCSSValue::GetBufferValue(mString), > + nsCSSValue::GetBufferValue(aOther.mString))); Rookie mistake: should be |NS_strcmp(...) == 0|. -_-

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d91354854339 Part 1: Preserve base URI on URLValueData objects. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/0549f87f0c29 Part 2: Add URLValueData comparison functions that work OMT. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/4de2d08774a1 Part 3: Use OMT-safe function for -moz-binding comparisons in CalcDifference. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd65026fd36 Part 4: Remove unused URLValueData comparison functions. r=emilio

https://hg.mozilla.org/mozilla-central/rev/d91354854339 https://hg.mozilla.org/mozilla-central/rev/0549f87f0c29 https://hg.mozilla.org/mozilla-central/rev/4de2d08774a1 https://hg.mozilla.org/mozilla-central/rev/2fd65026fd36