If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

style struct CalcDifference methods are not all safe to call off the main thread

RESOLVED FIXED in Firefox 51

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

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

Attachments

(4 attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 2

a year ago
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.)
(Assignee)

Comment 3

a year ago
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).
(Assignee)

Comment 5

a year ago
(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.
(Assignee)

Updated

a year ago
Depends on: 1298768
(Assignee)

Updated

a year ago
Depends on: 1298774
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d63e9eb6754e
Assignee: nobody → cam
Status: NEW → ASSIGNED

Comment 11

a year ago
mozreview-review
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 12

a year ago
mozreview-review
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?
Attachment #8785911 - Flags: review?(ecoal95) → review+

Comment 13

a year ago
mozreview-review
Comment on attachment 8785912 [details]
Bug 1297963 - Part 2: Add URLValueData comparison functions that work OMT.

https://reviewboard.mozilla.org/r/74924/#review72870
Attachment #8785912 - Flags: review?(ecoal95) → review+

Comment 14

a year ago
mozreview-review
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
Attachment #8785913 - Flags: review?(ecoal95) → review+

Comment 15

a year ago
mozreview-review
Comment on attachment 8785914 [details]
Bug 1297963 - Part 4: Remove unused URLValueData comparison functions.

https://reviewboard.mozilla.org/r/74928/#review72874
Attachment #8785914 - Flags: review?(ecoal95) → review+
(Assignee)

Comment 16

a year ago
mozreview-review-reply
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.

Comment 17

a year ago
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
(Assignee)

Comment 20

a year ago
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.)
(Assignee)

Comment 21

a year ago
mozreview-review
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|. -_-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=613957079019

Comment 27

a year ago
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

Comment 28

a year ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.