1202095 - inIDOMUtils.parseStyleSheet re-evaluates @import

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Tom Tromey :tromey]

While working on bug 984880 I noticed that parseStyleSheet re-evaluates
@import rules.  This causes problems for the style editor and for the
as-authored project.

Suppose there is an outer style sheet A that @imports an inner sheet B.
Open the style editor and edit B.
Then, edit A.
The edits you've made to B are now lost.

I think what we'd like for devtools is for CSSStyleSheet::ParseSheet to
try to preserve @imports rather than re-evaluate them.  The basic idea
would be to keep all the imported sheets when disconnecting, then reconnect
a sheet when the appropriate @import is seen.  This approach would make
it possible for parseStyleSheet to add or delete an @import, while avoiding
extra re-evals.

I have no idea how feasible this is.

Comment 1

[Tom Tromey :tromey]

Attachment: re-use @imported style sheets from inIDOMUtils.parseStyleSheet

Comment 2

[Tom Tromey :tromey]

Comment on attachment 8658379 [details] [diff] [review]
re-use @imported style sheets from inIDOMUtils.parseStyleSheet

This seems to work; though I am not sure if there are any details
I have missed.

Comment 3

[Cameron McCormack (:heycam)]

Some comments without my having had a very close look at the patch.

I think the general approach of saving style sheets to re-use should be OK.

I guess the expectation is that if you add a new @import to the sheet, then it will get loaded (and maybe cause other loads of its own, due to @import rules), but that if you leave existing @imports in there, with their same URL, those ones get re-used.

I'm worried that Loader methods might be called re-entrantly -- maybe one of the @import rules references a data: URL which is loaded synchronously? -- and so having "global" state in Loader::mReusableSheets might not be appropriate.  Or would that be fine/expected?  If you add a new @import rule, with a new URL, and that newly referenced sheet wants to @import one of the sheets that you had saved for re-use, would you want it using it?  Probably not -- I think you only want the top sheet that is being re-parsed to be able to re-use sheets.

Another option could instead be passing the sheets to re-use as an argument into nsCSSParser::ParseSheet.  Then, in CSSParserImpl::ProcessImport it can maybe pass in the re-usable sheet into Loader::LoadChildSheet.  So the CSSParserImpl would be in charge of ensuring each re-usable sheet gets re-used once.

Does that sound reasonable?

Comment 4

[Tom Tromey :tromey]

(In reply to Cameron McCormack (:heycam) from comment #3)
> Some comments without my having had a very close look at the patch.

Thanks very much.

> I'm worried that Loader methods might be called re-entrantly -- maybe one of
> the @import rules references a data: URL which is loaded synchronously? --
> and so having "global" state in Loader::mReusableSheets might not be
> appropriate.  Or would that be fine/expected?  If you add a new @import
> rule, with a new URL, and that newly referenced sheet wants to @import one
> of the sheets that you had saved for re-use, would you want it using it? 
> Probably not -- I think you only want the top sheet that is being re-parsed
> to be able to re-use sheets.

Yes, I agree, only the top sheet should be able to reuse the saved sheets.

> Another option could instead be passing the sheets to re-use as an argument
> into nsCSSParser::ParseSheet.  Then, in CSSParserImpl::ProcessImport it can
> maybe pass in the re-usable sheet into Loader::LoadChildSheet.  So the
> CSSParserImpl would be in charge of ensuring each re-usable sheet gets
> re-used once.
> 
> Does that sound reasonable?

Yes.  I'll do that.

Comment 5

[Tom Tromey :tromey]

Attachment: re-use @imported style sheets from inIDOMUtils.parseStyleSheet

Updated per comments.

Comment 6

[Tom Tromey :tromey]

Comment on attachment 8660038 [details] [diff] [review]
re-use @imported style sheets from inIDOMUtils.parseStyleSheet

This implements the idea you mentioned.

Note that a given sheet can only be reused once due to how
FindReusableStyleSheet modifies the list of style sheets
when one is found.

This messes up the alignment of some parameter names in nsCSSParser.
It seemed better this way to me, but I'm happy to change them all
to be aligned again if you prefer.

I chose to use a default argument to make the patch
simpler, but it's easy enough to update all the other callers to pass
nullptr if that is what you like.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8660038 [details] [diff] [review]
re-use @imported style sheets from inIDOMUtils.parseStyleSheet

Review of attachment 8660038 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these comments addressed.

::: layout/inspector/tests/test_parseStyleSheetImport.html
@@ +44,5 @@
> +     "re-re-re-parsed sheet has @import rule");
> +  isnot(sheet.cssRules[0].styleSheet, importedSheet,
> +     "imported sheet has changed now");
> +
> +  SimpleTest.finish();

Can you add a sub-test that has multiple @imports, with different URLs, possible added back in different orders, to check that that works as expected?

::: layout/style/CSSStyleSheet.cpp
@@ +2306,5 @@
> +      importRule->GetStyleSheet(getter_AddRefs(childSheet));
> +
> +      nsRefPtr<CSSStyleSheet> cssSheet = do_QueryObject(childSheet);
> +      if (cssSheet) {
> +        reusableSheets.AddReusableSheet(cssSheet);

CSSStyleSheet::GetOriginalURI says it can return null sometimes.  Should we check that and skip adding it to the reusable sheets list if so?

::: layout/style/CSSStyleSheet.h
@@ +251,5 @@
>  
>    bool UseForPresentation(nsPresContext* aPresContext,
>                              nsMediaQueryResultCacheKey& aKey) const;
>  
> +  nsresult ParseSheet(const nsAString& aInput,

While you're here, what do you think about renaming ParseSheet to ReparseSheet?

::: layout/style/Loader.cpp
@@ +516,5 @@
> +                                                  nsRefPtr<CSSStyleSheet>& aResult)
> +{
> +  for (size_t i = 0; i < mReusableSheets.Length(); ++i) {
> +    bool sameURI;
> +    nsresult rv = aURL->Equals(mReusableSheets[i]->GetOriginalURI(), &sameURI);

CSSStyleSheet::GetOriginalURL says it can return null sometimes.  Can we make this

@@ +519,5 @@
> +    bool sameURI;
> +    nsresult rv = aURL->Equals(mReusableSheets[i]->GetOriginalURI(), &sameURI);
> +    if (!NS_FAILED(rv) && sameURI) {
> +      aResult = mReusableSheets[i];
> +      mReusableSheets.RemoveElementAt(i);

Storing mReusableSheets in reverse order would also allow us to stop shuffling elements around so much as in the common case of the @import rules remaining the same, we'll pick the element off the end of the array.

::: layout/style/Loader.h
@@ +143,5 @@
> +  }
> +
> +  ~LoaderReusableStyleSheets()
> +  {
> +  }

May as well omit the destructor.

@@ +148,5 @@
> +
> +  /**
> +   * Look for a reusable sheet (see AddReusableSheet) matching the
> +   * given URL.  If found, set aResult, remove the reused sheet from
> +   * the internal list, and return true.  Otherwise, return false.

Mention explicitly that aResult remains untouched if false is returned?

@@ +158,5 @@
> +
> +  /**
> +   * Indicate that a certain style sheet is available for reuse if its
> +   * URI matches the URI of an @import.  Sheets should be added in the
> +   * opposite order in which they are intended to be reused.

I assume we're adding in reverse order since that's what's easiest for CSSStyleSheet::ParseSheet.  To avoid moving the contents of the array each time AddReusableSheet is called, how about appending them to mReusableSheets and then iterating over them in reverse order?

::: layout/style/nsCSSParser.cpp
@@ +1178,5 @@
>    // Used for @import rules
>    mozilla::css::Loader* mChildLoader; // not ref counted, it owns us
>  
> +  // Any sheets we may reuse when parsing an @import.
> +  LoaderReusableStyleSheets* mReusableSheets;

Please init this to null in the CSSParserImpl constructor.

::: layout/style/nsCSSParser.h
@@ +88,5 @@
>                        nsIURI*          aBaseURI,
>                        nsIPrincipal*    aSheetPrincipal,
>                        uint32_t         aLineNumber,
> +                      bool             aAllowUnsafeRules,
> +                      mozilla::css::LoaderReusableStyleSheets* aReusableSheets = nullptr);

I do have mixed feelings about identifier alignment like this, but don't mind leaving long types to break the pattern.  Though I think more important is to keep to 80 columns.  Can you rewrap the "nullptr);" to the next line?

Comment 8

[Tom Tromey :tromey]

(In reply to Cameron McCormack (:heycam) from comment #7)

> Can you add a sub-test that has multiple @imports, with different URLs,
> possible added back in different orders, to check that that works as
> expected?

Thanks, this revealed a bug.

> While you're here, what do you think about renaming ParseSheet to
> ReparseSheet?

Aha, I didn't realize that domutils was the only user of this method.
This change let me drop the new enum.

Comment 9

[Tom Tromey :tromey]

Attachment: re-use @imported style sheets from inIDOMUtils.parseStyleSheet

This addresses the review comments.

I removed the enum that I added in an earlier version of the patch.
Because ReparseSheet is only called from a single place, the new
behavior doesn't need to be optional.

I changed LoaderReusableStyleSheets to store mReusableSheets as
suggested.  This required a little oddity in the iteration, to avoid a
compiler warning due to comparing "size_t >= 0".

I changed it to check for a null GetOriginalURI.  I added some
assertions in Loader.cpp as well.  These should never fire; one
because the caller of FindReusableStyleSheet has the same assertion,
and the other due to the check done when constructing the list of
reusable sheets.

The test to reorder the imported sheets revealed that nothing was
resetting a CSSStyleSheet's mNext link; causing AppendStyleSheet to
create a circular list. This version of the patch clears mNext when
clearing the child sheet list.

Comment 10

[Tom Tromey :tromey]

Attachment: re-use @imported style sheets from inIDOMUtils.parseStyleSheet

Forgot to git add the new .css file for the test.

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8662428 [details] [diff] [review]
re-use @imported style sheets from inIDOMUtils.parseStyleSheet

I wasn't sure whether this changed enough to warrant re-review (in
particular due to the enum removal and mNext resetting), or if
I could carry over the r+.  So, erring on the side of caution.
Comment #9 explains the changes.

Comment 12

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ce24efcd592

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8662428 [details] [diff] [review]
re-use @imported style sheets from inIDOMUtils.parseStyleSheet

Review of attachment 8662428 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Can you just add one more subtest (that I just thought of) for adding two @imports with the same URL and ensuring that one gets re-used and the other re-loaded.

::: layout/style/Loader.cpp
@@ +516,5 @@
> +                                                  nsRefPtr<CSSStyleSheet>& aResult)
> +{
> +  MOZ_ASSERT(aURL);
> +  for (size_t i = mReusableSheets.Length(); i > 0; --i) {
> +    size_t index = i - 1;

Yeah, I'm never sure what's the most idiomatic way to loop downwards over an unsigned variable.  I've never much liked the too-cute:

  for (size_t i = n; i-- > 0; )

This isn't bad:

  for (size_t i = n - 1; i < n; --i)

though I think the |i < n| condition takes a little longer to think about than I'd like.

Your approach is fine. :)

Comment 14

[Tom Tromey :tromey]

Attachment: re-use @imported style sheets from inIDOMUtils.parseStyleSheet

Add new test.

Comment 15

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f17fdcec3dfb

Comment 16

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4eba88b020f

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4596456439

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0d4596456439