[Stylo] Crash when opening devtool Inspector

RESOLVED FIXED in Firefox 55

Status

()

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

People

(Reporter: shinglyu, Assigned: bradwerth)

Tracking

unspecified
mozilla55
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Created attachment 8834314 [details]
talos_g2.log

Stylo does not run on Talos's g2 test, which tests devtool startup time. You can run `./mach talos-test --suite g2` to test it. 

Or to reproduce it manually
* Open Stylo
* Load www.google.com
* Open the devtool by clicking F12
* Switch to the "Inspector" tab => Crash

Please see the attached talos log for the error.
(Reporter)

Comment 1

a year ago
Brad, I heard that you are working on the devtool, maybe you have some insight on this?
Flags: needinfo?(bwerth)
(Assignee)

Comment 2

a year ago
Yep, I'll figure it out.
Assignee: nobody → bwerth
Flags: needinfo?(bwerth)
(Assignee)

Comment 3

a year ago
Working for me on OS X with build 601e7119c08c. At least, the manual steps don't crash, and the talos test appears to run adequately (lots of pages loaded, no obvious crashes). Still failing for you?
Flags: needinfo?(slyu)
(Reporter)

Comment 4

a year ago
Created attachment 8835834 [details]
talos_g2.log

I pull the latest code, now the manual way doesn't cause a crash, but talos-g2 still don't run. I've attached the log for your reference.

More specifically I believe this is the problem: 

11:06:59     INFO -  PROCESS | 21952 | console.error:
11:06:59    ERROR -  PROCESS | 21952 |   Message: TypeError: doc.documentElement is null
11:06:59     INFO -  PROCESS | 21952 |   Stack:
11:06:59     INFO -  PROCESS | 21952 |     supportsHighlighters@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:2874:9
Attachment #8834314 - Attachment is obsolete: true
Flags: needinfo?(slyu) → needinfo?(bwerth)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
I still can't replicate, but attachment 8836948 [details] will make loader.js survive a document with a missing root element, or at least not fail at the same spot.
Flags: needinfo?(bwerth)
(Assignee)

Updated

a year ago
Attachment #8836948 - Flags: review?(slyu)
(Reporter)

Comment 7

a year ago
Now I know the problem: 
1. The "Computed" style tab under Inspector is triggering crashes because it try to interpret servo-flavor rule node as gecko-flavor. 
2. The Style Editor fails to load the list of stylesheets

I worked around them by skipping in various places in the devtool server, but we should probably make them work with stylo anyway, so I'm not going to merge the patch right now. We need to fix those places and make them read real stylo CSS values.
(Reporter)

Comment 8

a year ago
Created attachment 8837482 [details] [diff] [review]
bug_1337305_workaround.patch

These are the places that causes crashes or hang. We should probably hook them up with stylo so the DevTool can work correctly.
Flags: needinfo?(bwerth)
(Reporter)

Comment 9

a year ago
mozreview-review
Comment on attachment 8836948 [details]
Bug 1337305 Part 1: Refactor GetRuleNodeForElement to just return the StyleContext, so caller can either get a RuleNode or a raw servo rule.

https://reviewboard.mozilla.org/r/112250/#review114080

Thanks for the work, but this is not the root cause of the issue so we probably shouldn't merge this.
Attachment #8836948 - Flags: review?(slyu) → review-
(Reporter)

Comment 10

a year ago
Brad, are you working on this now? If not would you mind me give it a try?
(Assignee)

Comment 11

a year ago
Of course, go for it. I'll check in with you when I'm ready to help.
Flags: needinfo?(bwerth)
(Reporter)

Comment 12

a year ago
Xidorn,

The GetCSSStyleRules in inDOMUtils.cpp expects to get a list of css::Rules, but the best we can get from stylo is Servo's StyleRule. You mentioned about construct new css::Rule just for devtools, could you kindly explain your idea further?

> 08:40	xidorn	shinglyu: i think one solution is to construct new css::Rule just for devtools
> 08:41	shinglyu	xidorn: That means a manual converting RuleNode to css::Rule?
> 08:41	xidorn	that says, I don't think we really need to ensure there is only one css:Rule to servo's rule
> 08:42	xidorn	so we can havr multiple
Flags: needinfo?(xidorn+moz)
My idea is that, first, we get a list of StyleRule (RawServoStyleRule in Gecko side) from Servo, then create a list of ServoStyleRule from those StyleRules like what is done in ServoCSSRuleList::GetRule [1], now we have a list of css::Rules.

ServoStyleRule is basically a wrapper type of RawServoStyleRule, so I guess it doesn't matter if we have multiple ServoStyleRule for a single RawServoStyleRule.

This would allow devtools to display the style data, but updating the style wouldn't trigger any restyle. To make updating work, we need their stylesheets and document, which Servo's StyleRule doesn't have. We may need to figure out a plan for that.


[1] https://dxr.mozilla.org/mozilla-central/rev/16effd5b21ab03629feca04b5b83911bb757394c/layout/style/ServoCSSRuleList.cpp#64-65
Flags: needinfo?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 16

a year ago
mozreview-review
Comment on attachment 8836948 [details]
Bug 1337305 Part 1: Refactor GetRuleNodeForElement to just return the StyleContext, so caller can either get a RuleNode or a raw servo rule.

https://reviewboard.mozilla.org/r/112250/#review117726

Looks good to me :) But I'm not very comfortable with reviewing Gecko, so you might want a second eye on this when you are ready to land.
Attachment #8836948 - Flags: review?(slyu) → review+
(Assignee)

Comment 17

a year ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)
> My idea is that, first, we get a list of StyleRule (RawServoStyleRule in
> Gecko side) from Servo, then create a list of ServoStyleRule from those
> StyleRules like what is done in ServoCSSRuleList::GetRule [1], now we have a
> list of css::Rules.

I need some more insight on how to do this. Within GetCSSStyleRules, I can see how to turn the nsStyleContext into a ComputedServoValues, but not into a RawServoStyleRule. Are you proposing a way to get a RawServoStyleRule from an Element using existing Element data?
Flags: needinfo?(xidorn+moz)
I thought you can get the RuleNode from Servo as well, but it seems I was wrong. So the problem is that Servo's ComputedValues do not have a pointer to RuleNode like Gecko. If we have a pointer from ComputedValues to RuleNode, we can get StyleSource from the node and extract rules it uses.

I'm not sure whether it is easy to add pointer from ComputedValues to RuleNode. emilio, any thoughts?
Flags: needinfo?(xidorn+moz) → needinfo?(emilio+bugs)
So, right now you can't get the Servo rule node from the nsStyleContext directly, since it just stores a pointer to the computed values. There's nothing essentially preventing us from doing that, I think, but is it needed?

It seems that GetCSSStyleRules has access to the Element. The element can access its rule node fairly easily getting the style data, see [1], for example. Would that do the job?

[1]: https://dxr.mozilla.org/mozilla-central/source/servo/ports/geckolib/glue.rs#1216
Flags: needinfo?(emilio+bugs)
(Assignee)

Comment 20

a year ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #19)

> It seems that GetCSSStyleRules has access to the Element. The element can
> access its rule node fairly easily getting the style data, see [1], for
> example. Would that do the job?

I'm not sure that will get us the information we need to construct a css::Rule. I'll pursue the idea of linking the ServoComputedValues back to the RawServoStyleRule, as proposed in comment 18.
(In reply to Brad Werth [:bradwerth] from comment #20)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #19)
> 
> > It seems that GetCSSStyleRules has access to the Element. The element can
> > access its rule node fairly easily getting the style data, see [1], for
> > example. Would that do the job?
> 
> I'm not sure that will get us the information we need to construct a
> css::Rule. I'll pursue the idea of linking the ServoComputedValues back to
> the RawServoStyleRule, as proposed in comment 18.

How wouldn't it? The rule node has access to all the style rules matched... AFAIK RawServoStyleRule would just be the Arc<> that is in RuleNode::style_source.
(Assignee)

Comment 22

a year ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #21)

> How wouldn't it? The rule node has access to all the style rules matched...
> AFAIK RawServoStyleRule would just be the Arc<> that is in
> RuleNode::style_source.

Perhaps I'm misunderstanding your guidance in comment 19. It seems you are directing me to use Servo_Element_GetSnapshot, is that right? I don't see how to turn a ServoElementSnapshot struct into a css::Rule or a RuleNode or a RawServoStyleRule, etc. I also don't see in GetCSSStyleRules where I can get a RuleNode that isn't a Gecko RuleNode.
Flags: needinfo?(emilio+bugs)
I was using it as an example of how to use the style data (ElementData), which lives in servo/components/style/data.rs

Basically, you can get the rule node with element.borrow_data().unwrap().styles().primary.rules.
Flags: needinfo?(emilio+bugs)
(Assignee)

Updated

a year ago
Blocks: 1346256
(Assignee)

Updated

a year ago
Attachment #8836948 - Flags: review?(xidorn+moz)
Attachment #8842229 - Flags: review?(xidorn+moz)
(Assignee)

Comment 24

a year ago
The Servo-side changes necessary to report computed styles (as opposed to merely not crash) are non-trivial. I've created Bug 134256 to continue the work.

Comment 25

a year ago
mozreview-review
Comment on attachment 8836948 [details]
Bug 1337305 Part 1: Refactor GetRuleNodeForElement to just return the StyleContext, so caller can either get a RuleNode or a raw servo rule.

https://reviewboard.mozilla.org/r/112250/#review121234

::: layout/inspector/inDOMUtils.h:31
(Diff revision 2)
> -  // aStyleContext must be released by the caller once he's done with aRuleNode.
> -  static nsresult GetRuleNodeForElement(mozilla::dom::Element* aElement,
> +  // aStyleContext must be released by the caller.
> +  static nsresult GetStyleContextForElement(mozilla::dom::Element* aElement,
> -                                        nsIAtom* aPseudo,
> +                                            nsIAtom* aPseudo,
> -                                        nsStyleContext** aStyleContext,
> +                                            nsStyleContext** aStyleContext);

Since it now only returns one object, could you just make it returning `already_AddRefed<nsStyleContext>`? It would make the ownership model much clearer from code itself, and callsite code can also be simplified.

It seems to me the callsite is never check its `nsresult` return value, so you can simply return `nullptr` in case of any error inside this function.
Attachment #8836948 - Flags: review?(xidorn+moz)

Comment 26

a year ago
mozreview-review
Comment on attachment 8842229 [details]
Bug 1337305 Part 2: Change GetCSSStyleRules to return RuleNodes for Gecko rules, and nothing otherwise (instead of asserting).

https://reviewboard.mozilla.org/r/116120/#review121236

::: layout/inspector/inDOMUtils.cpp:247
(Diff revision 1)
>      return NS_OK;
>    }
>  
> +  NonOwningStyleContextSource source = styleContext->StyleSource();
> +  if (!source.IsNull() && source.IsGeckoRuleNodeOrNull()) {
> -  nsRuleNode* ruleNode = styleContext->RuleNode();
> +    nsRuleNode* ruleNode = styleContext->RuleNode();

Probably better `source.AsGeckoRuleNode()` which may reduce an unnecessary release check. But I guess it doesn't matter a lot.
Attachment #8842229 - Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

a year ago
mozreview-review
Comment on attachment 8842229 [details]
Bug 1337305 Part 2: Change GetCSSStyleRules to return RuleNodes for Gecko rules, and nothing otherwise (instead of asserting).

https://reviewboard.mozilla.org/r/116120/#review121610

::: layout/inspector/inDOMUtils.cpp:247
(Diff revision 1)
>      return NS_OK;
>    }
>  
> +  NonOwningStyleContextSource source = styleContext->StyleSource();
> +  if (!source.IsNull() && source.IsGeckoRuleNodeOrNull()) {
> -  nsRuleNode* ruleNode = styleContext->RuleNode();
> +    nsRuleNode* ruleNode = styleContext->RuleNode();

AsGeckoRuleNode contains internal asserts that would fail if the source can't be cast to nsRuleNode. Since that's the exact case we're trying to cover in the if, we need logic like this.

Comment 30

a year ago
mozreview-review
Comment on attachment 8836948 [details]
Bug 1337305 Part 1: Refactor GetRuleNodeForElement to just return the StyleContext, so caller can either get a RuleNode or a raw servo rule.

https://reviewboard.mozilla.org/r/112250/#review121756

r=me with the issues below addressed.

::: layout/inspector/inDOMUtils.h:31
(Diff revision 3)
>    inDOMUtils();
>  
>  private:
>    virtual ~inDOMUtils();
>  
> -  // aStyleContext must be released by the caller once he's done with aRuleNode.
> +  // aStyleContext must be released by the caller.

This comment is unnecessary now. `already_AddRefed` would assert if the pointer it contains is not moved away.

::: layout/inspector/inDOMUtils.cpp:237
(Diff revision 3)
> -  RefPtr<nsStyleContext> styleContext;
> -  GetRuleNodeForElement(element, pseudoElt, getter_AddRefs(styleContext), &ruleNode);
> -  if (!ruleNode) {
> +  RefPtr<nsStyleContext> styleContext = GetCleanStyleContextForElement(
> +    element,
> +    pseudoElt);

Probably more readable this way:
```c++
  RefPtr<nsStyleContext> styleContext =
    GetCleanStyleContextForElement(element, pseudoElt);
```
Attachment #8836948 - Flags: review?(xidorn+moz) → review+

Comment 31

a year ago
mozreview-review-reply
Comment on attachment 8842229 [details]
Bug 1337305 Part 2: Change GetCSSStyleRules to return RuleNodes for Gecko rules, and nothing otherwise (instead of asserting).

https://reviewboard.mozilla.org/r/116120/#review121610

> AsGeckoRuleNode contains internal asserts that would fail if the source can't be cast to nsRuleNode. Since that's the exact case we're trying to cover in the if, we need logic like this.

Probably I was not clear enough. I was not asking you to remove the check in the line above, but just replacing `styleContext->RuleNode()` to `source.AsGeckoRuleNode()`. It should be safe since you have checked `source.IsGeckoRuleNodeOrNull()`.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 34

a year ago
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a6deabb82919
Part 1: Refactor GetRuleNodeForElement to just return the StyleContext, so caller can either get a RuleNode or a raw servo rule. r=shinglyu,xidorn
https://hg.mozilla.org/integration/autoland/rev/d83cf405f54c
Part 2: Change GetCSSStyleRules to return RuleNodes for Gecko rules, and nothing otherwise (instead of asserting). r=xidorn

Comment 35

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a6deabb82919
https://hg.mozilla.org/mozilla-central/rev/d83cf405f54c
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.