Closed Bug 1290214 Opened 3 years ago Closed 3 years ago

stylo: Stop generating so many backtraces during stylo runs

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(8 files)

Right now we have a bunch of NS_ASSERTION and NS_ERROR uses that cause backtraces to be generated during debug runs. At least on Mac, these cause considerable slowdown and spam the console.

I'm going through and cleaning up the various sites that I hit, filing bugs as appropriate.
In practice I'm pretty sure these cases wouldn't have a content docshell anyway.
We probably don't need more robust machinery here, since eventually we'll just
make stylo pref-based for every new prescontext regardless of type.
Attachment #8775793 - Flags: review?(ealvarez)
Attachment #8775794 - Flags: review?(cam) → review?(ealvarez)
Attachment #8775795 - Flags: review?(cam) → review?(ealvarez)
We do all the work we need to do with the snapshot model in AttributeWillChange.
Attachment #8775797 - Flags: review?(ealvarez)
These are core methods that we know we need to implement, and I'm not worried that
we'll forget about them. The warnings should be enough here.
Attachment #8775800 - Flags: review?(ealvarez)
Attachment #8775793 - Flags: review?(ealvarez) → review+
Attachment #8775796 - Flags: review?(ealvarez) → review+
Attachment #8775794 - Flags: review?(ealvarez) → review+
Attachment #8775795 - Flags: review?(ealvarez) → review+
Comment on attachment 8775797 [details] [diff] [review]
Part 5 - Make ServoRestyleManager::AttributeChanged a no-op. v1

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

::: layout/base/ServoRestyleManager.h
@@ +63,5 @@
>                             int32_t aModType,
>                             const nsAttrValue* aNewValue);
>    void AttributeChanged(dom::Element* aElement, int32_t aNameSpaceID,
>                          nsIAtom* aAttribute, int32_t aModType,
> +                        const nsAttrValue* aOldValue) {}

Could you add a TODO comment here? IIRC not all AttributeChanged come after AttributeWillChange so we should do something there (even if it's just fixing the callers).

We should double-check it. Probably the easiest way to do it is add an assertion that the old value is already in the snapshot.

r=me with that.
Attachment #8775797 - Flags: review?(ealvarez) → review+
Attachment #8775798 - Flags: review?(ealvarez) → review+
Attachment #8775799 - Flags: review?(ealvarez) → review+
Comment on attachment 8775800 [details] [diff] [review]
Part 8 - Remove NS_ERROR for {un,partially-}implemented ServoRestyleManager and ServoStyleSet methods. v1

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

::: layout/style/ServoStyleSet.cpp
@@ +377,5 @@
>  nsRestyleHint
>  ServoStyleSet::HasStateDependentStyle(dom::Element* aElement,
>                                        EventStates aStateMask)
>  {
> +  NS_WARNING("stylo: HasStateDependentStyle always returns zero!");

LGTM. Just for the record, these are something we might not need after all (it's called from ContentStateChanged, and we defer the check for the style hints computation).
Attachment #8775800 - Flags: review?(ealvarez) → review+
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56e69ffed4a9
Stop asserting when we don't have enough information to use the servo backend. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/5980af5a8eb7
Remove NS_ERRORs in css::Loader. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/31aaf3f16d98
Remove NS_ERROR in the case where we skip checking the stylesheet service. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfe87f58256d
Remove NS_ERROR for media queries and @font-face. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/23416e612134
Make ServoRestyleManager::AttributeChanged a no-op. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b5991d2f46e
Remove NS_ERROR for XBL stylesheet management. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff91f71e37b
Remove NS_ERROR for unhandled document state changes. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/5224dd4c0379
Remove NS_ERROR for {un,partially-}implemented ServoRestyleManager and ServoStyleSet methods. r=emilio
You need to log in before you can comment on or make changes to this bug.