Closed Bug 1374181 Opened 7 years ago Closed 7 years ago

EnsureUniqueInner may be called before sheet is complete because of EnsureUniqueInnerOnCSSSheets invoked by DevTools

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: xidorn, Assigned: bradwerth)

References

Details

Attachments

(1 file)

Bug 1374181 Part 1: Don't attempt EnsureUniqueInner on incomplete sheets in a StyleSet.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
This can be checked from the following steps on a debug build:
1. open Inspector in DevTools
2. visit https://test1.upsuper.org/delayed-style/

This would crash the debug build due to
> Assertion failure: !data->mSheet->IsModified() (should not get marked modified during parsing)

The test page uses a service worker to simulate delayed loading from an import rule.

This affects both Stylo and Gecko. The related code is in {nsStyleSet,ServoStyleSet}::EnsureUniqueInnerOnCSSSheets. This function should check whether a sheet is complete before invoking EnsureUniqueInner.

I'm not sure whether changing those functions is enough. We may also need to do something around mNeedsRestyleAfterEnsureUniqueInner to ensure the data gets updated when the sheet becomes complete.
Priority: -- → P3
Brad: I'm marking this 'affected' for FF57. Please have a look and let us know if we can safely plug this crash for FF57. Thx!
status-firefox57: --- → affected
status-firefox58: --- → affected
Flags: needinfo?(bwerth)
Can't reproduce. It seems most likely that Bug 1395322 resolved this.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(bwerth)
Resolution: --- → WORKSFORME
No, this is not fixed.

To reproduce this issue, you need to follow the order in comment 0 that you need to open the Inspector *before* navigating to that page. And you need to wait for about 5s until the page load finishes.
Status: RESOLVED → REOPENED
Flags: needinfo?(bwerth)
Resolution: WORKSFORME → ---
Assignee: nobody → bwerth
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0)
> I'm not sure whether changing those functions is enough. We may also need to
> do something around mNeedsRestyleAfterEnsureUniqueInner to ensure the data
> gets updated when the sheet becomes complete.

Attachment 8913835 [details] resolves the crash, but I need to create something additional to handle this issue. With just Part 1, dev tools view of Rules is not updated when the delayed stylesheet is applied.
Flags: needinfo?(bwerth)
Blocks: 1404538
I decided to open a follow-up Bug 1404538, so this crash can be resolved now.
Attachment #8913835 - Flags: review?(xidorn+moz)
Comment on attachment 8913835 [details]
Bug 1374181 Part 1: Don't attempt EnsureUniqueInner on incomplete sheets in a StyleSet.

https://reviewboard.mozilla.org/r/185216/#review190272

::: layout/style/ServoStyleSet.cpp:1282
(Diff revision 1)
> +    // We only proceed with complete sheets, to avoid marking them as dirty
> +    // while they are still being parsed. This is acceptable since incomplete
> +    // sheets have unique inners.
> +
> +    // While we're here, check this invariant.
> +    MOZ_ASSERT(sheet->IsComplete() || sheet->HasUniqueInner());

I doubt this... What would happen if I put two import rules with the same url there?
I've updated my testcase to include two @import rules. You can test with your patch and see if the assertion is valid with that.
Comment on attachment 8913835 [details]
Bug 1374181 Part 1: Don't attempt EnsureUniqueInner on incomplete sheets in a StyleSet.

https://reviewboard.mozilla.org/r/185216/#review190798

::: layout/style/ServoStyleSet.cpp:1282
(Diff revision 1)
> +    // We only proceed with complete sheets, to avoid marking them as dirty
> +    // while they are still being parsed. This is acceptable since incomplete
> +    // sheets have unique inners.
> +
> +    // While we're here, check this invariant.
> +    MOZ_ASSERT(sheet->IsComplete() || sheet->HasUniqueInner());

You're right. I'll need to find another approach.
Comment on attachment 8913835 [details]
Bug 1374181 Part 1: Don't attempt EnsureUniqueInner on incomplete sheets in a StyleSet.

https://reviewboard.mozilla.org/r/185216/#review190986

::: commit-message-cd9c8:1
(Diff revision 2)
> +Bug 1374181 Part 1: Do not mark incomplete sheets as dirty in EnsureUniqueInner().

Please explain in the commit message or (preferably) the comment why this change is fine.

My guess would be, since inspector only cares about style context and its related rule nodes, which means an incompelte stylesheet wouldn't ever show up in it, and thus it doesn't matter whether they have unique inner.

In that case, I suppose it makes more sense to do the skip in `EnsureUniqueInnerOnCSSSheets` rather than in `EnsureUniqueInner`.

Also it would be great if there could be a new testcase, since it is something which can happen in practice when the network is slow. But it is probably doesn't matter that much.
Attachment #8913835 - Flags: review?(xidorn+moz)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #13)
> Please explain in the commit message or (preferably) the comment why this
> change is fine.
> 
> My guess would be, since inspector only cares about style context and its
> related rule nodes, which means an incompelte stylesheet wouldn't ever show
> up in it, and thus it doesn't matter whether they have unique inner.
> 
> In that case, I suppose it makes more sense to do the skip in
> `EnsureUniqueInnerOnCSSSheets` rather than in `EnsureUniqueInner`.

I think I agree with this. Overall, I'm still puzzling through the right way to solve this. I'm going to lay out all the interlocking parts that I can find in an effort to help me be more confident on the proposed solution:

1) Incomplete sheets must not modify their inners. This is enforced by having almost all callers of WillDirty() assert or throw exceptions if the sheet is incomplete. One exception is StyleSheet::PrependStyleSheet, which works on incomplete sheets but doesn't change the inner. There are other functions which probably should enforce this invariant, but don't:
* CSSStyleSheet::AppendStyleRule
* CSSStyleSheet::DeleteRuleInternal
* MediaList::DoMediaChange
* DOMCSSDeclarationImpl::GetCSSDeclaration -- this can only come after a successful EnsureSafeToHandoutCSSRules, so probably fine.
In all these cases, we might be able to get more clarity by renaming mDirty into mInnerDirty, since that's what it actually means. Possibly this flag should even move into the inner itself.

2) EnsureUniqueInner marks an inner as modified, even if nothing is actually changed (if inner is already unique). I'm not clear why this is considered correct. DidDirty does check for IsModified, and that could be relaxed to account for the cases where WillDirty early exits. Changing this behavior would not solve the issue for the testcase which delay loads two stylesheets that share an inner.

3) Getting DOM CSSRule objects from a sheet requires a unique inner. This makes sense, but we could relax it for incomplete sheets as Xidorn has proposed in the bug description and in comment 13. Bug 1404538 has been filed to fix this up for devtools however we choose to relax invariants in this bug.

4) When a sheet is eventually loaded and marked complete, the inner must be unmodified. This makes sense and probably has to stay since any sheet that was prematurely cloned would not be able to find its clone to update once the sheet was complete. 

Okay, I'd like to make a lot of the changes laid out here (renaming and/or moving mDirty, relaxing asserts in DidDirty) just for purposes of clarity, but I don't want that to complicate this bug. I'll make a version of the patch that puts the changed logic and comments in EnsureUniqueInnerOnCSSSheets, and file a new bug to follow up on some of these other ideas.

> Also it would be great if there could be a new testcase, since it is
> something which can happen in practice when the network is slow. But it is
> probably doesn't matter that much.

I'll attempt to modify the https://test1.upsuper.org/delayed-style/ testcase into a proper test. The moving parts (opening devtools, invoking service workers) make it a more complex test than I've created before. I'll try to figure it out.
Blocks: 1405427
The approach in attachment 8913835 [details] causes test layout/inspector/tests/test_bug536379.html to fail. More is needed.
(In reply to Brad Werth [:bradwerth] from comment #18)
> The approach in attachment 8913835 [details] causes test
> layout/inspector/tests/test_bug536379.html to fail. More is needed.

That's embarassing; I had the logic reversed.
(In reply to Brad Werth [:bradwerth] from comment #14)
> 1) Incomplete sheets must not modify their inners. This is enforced by
> having almost all callers of WillDirty() assert or throw exceptions if the
> sheet is incomplete. One exception is StyleSheet::PrependStyleSheet, which
> works on incomplete sheets but doesn't change the inner. There are other
> functions which probably should enforce this invariant, but don't:
> * CSSStyleSheet::AppendStyleRule

It must not. nsCSSParser uses this method to fill a stylesheet, and that is actually the only place this method is called. If it requires the stylesheet to be complete to parse CSS into stylesheet, the stylesheet would never be able to be complete.

> * CSSStyleSheet::DeleteRuleInternal

It is not necessary. This method is only called from StyleSheet::DeleteRule, and the later would early return if the stylesheet is incomplete.

> * MediaList::DoMediaChange

This is not necessary either. A media list can only present in a complete stylesheet (or a stylesheet to be completed, that the parser is still filling). Since this is only invoked via CSSOM, at which moment, if you can get a media list, you should already have a complete stylesheet (half-filled stylesheet shouldn't be accessible from content).

> * DOMCSSDeclarationImpl::GetCSSDeclaration -- this can only come after a
> successful EnsureSafeToHandoutCSSRules, so probably fine.

Similar to MediaList::DoMediaChange, you can only get this object from a complete stylesheet.

> In all these cases, we might be able to get more clarity by renaming mDirty
> into mInnerDirty, since that's what it actually means. Possibly this flag
> should even move into the inner itself.

So none of the cases above is a real problem. And no, mDirty doesn't mean mInnerDirty.

Specifically, a stylesheet is marked dirty as soon as its rule list is handed to script, after which point, we are not able to make the inner unique anymore (because we don't have mechanism to change what object does script hold). Because of that, a stylesheet can be marked dirty even if nothing has touched its inner.

The term "dirty" probably doesn't make much sense given this, but it doesn't come to me any better naming at the moment.

Whether mDirty is in stylesheet or inner doesn't really matter, though, because when dirty flag is set, inner and stylesheet must have a one-to-one mapping, and a dirty stylesheet / inner can never be turned back to a non-dirty one.

> 2) EnsureUniqueInner marks an inner as modified, even if nothing is actually
> changed (if inner is already unique). I'm not clear why this is considered
> correct. DidDirty does check for IsModified, and that could be relaxed to
> account for the cases where WillDirty early exits. Changing this behavior
> would not solve the issue for the testcase which delay loads two stylesheets
> that share an inner.

As mentioned above, handing any object to script would make us lose the ability to make inner unique, so we need to ensure uniqueness before and after that.

> 3) Getting DOM CSSRule objects from a sheet requires a unique inner. This
> makes sense, but we could relax it for incomplete sheets as Xidorn has
> proposed in the bug description and in comment 13. Bug 1404538 has been
> filed to fix this up for devtools however we choose to relax invariants in
> this bug.

I'm not proposing that.

Saying that we can skip incomplete stylesheet in EnsureUniqueInnerOnCSSSheets is purely based on the assumption that we will *not* get any rule object from incomplete stylesheets in the callsites of that function, and thus we would *not* hand any of those objects to script.

> 4) When a sheet is eventually loaded and marked complete, the inner must be
> unmodified. This makes sense and probably has to stay since any sheet that
> was prematurely cloned would not be able to find its clone to update once
> the sheet was complete. 

I'm assuming you mean any inner (rather than sheet) that was prematurely cloned would not be able to find its clone. That is true. Incomplete sheets, on the other hand, can be cloned with a shared incomplete inner. They just cannot ensure unique inner.

> Okay, I'd like to make a lot of the changes laid out here (renaming and/or
> moving mDirty, relaxing asserts in DidDirty) just for purposes of clarity,
> but I don't want that to complicate this bug. I'll make a version of the
> patch that puts the changed logic and comments in
> EnsureUniqueInnerOnCSSSheets, and file a new bug to follow up on some of
> these other ideas.
> 
> > Also it would be great if there could be a new testcase, since it is
> > something which can happen in practice when the network is slow. But it is
> > probably doesn't matter that much.
> 
> I'll attempt to modify the https://test1.upsuper.org/delayed-style/ testcase
> into a proper test. The moving parts (opening devtools, invoking service
> workers) make it a more complex test than I've created before. I'll try to
> figure it out.

Service workers is not necessary. It is just used for simulating a delayed load. I think you can use sjs with a delayed response like what is done in inspector-delay-image-response.sjs [1]. Since this test requires devtools to be in effect before the stylesheet is loaded, a timeout-based approach may be a source of indeterminacy, but I have no better idea.

[1] https://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/devtools/server/tests/mochitest/inspector-delay-image-response.sjs
Comment on attachment 8913835 [details]
Bug 1374181 Part 1: Don't attempt EnsureUniqueInner on incomplete sheets in a StyleSet.

https://reviewboard.mozilla.org/r/185216/#review191306

r=me with the part of comment removed in both files.

::: layout/style/ServoStyleSet.cpp:1288
(Diff revision 5)
> +    // The only issue is that requesters of CSSRules might also want to know
> +    // if those rules are complete, and to listen for completion if the rules
> +    // are incomplete. Bug 1404538 will fix this up at least for devtools, and
> +    // possibly for other JS callers of the cssRules property.

It doesn't seem to me this part of comment is relevant to the code here. Probably move it to the commit message instead.

Also, I don't think this is really a problem. Pages with chrome priviledge can listen to `StyleSheetAdded` event, or just listen to `load` event of `<link>` if it's a top level stylesheet (rather than `@import`).
Attachment #8913835 - Flags: review?(xidorn+moz) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eaddde278c9f
Part 1: Don't attempt EnsureUniqueInner on incomplete sheets in a StyleSet. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/eaddde278c9f
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: