Shing Lyu [:shinglyu] Reporter
	Description • 7 years ago

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.

	Shing Lyu [:shinglyu] Reporter
	Comment 1 • 7 years ago

Brad, I heard that you are working on the devtool, maybe you have some insight on this?

	Brad Werth [:bradwerth] Assignee
	Comment 2 • 7 years ago

Yep, I'll figure it out.

	Brad Werth [:bradwerth] Assignee
	Comment 3 • 7 years 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?

	Shing Lyu [:shinglyu] Reporter
	Comment 4 • 7 years ago

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

	Brad Werth [:bradwerth] Assignee
	Comment 6 • 7 years 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.

	Shing Lyu [:shinglyu] Reporter
	Comment 7 • 7 years 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.

	Shing Lyu [:shinglyu] Reporter
	Comment 8 • 7 years ago

These are the places that causes crashes or hang. We should probably hook them up with stylo so the DevTool can work correctly.

	Shing Lyu [:shinglyu] Reporter
	Comment 9 • 7 years 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.

	Shing Lyu [:shinglyu] Reporter
	Comment 10 • 7 years ago

Brad, are you working on this now? If not would you mind me give it a try?

	Brad Werth [:bradwerth] Assignee
	Comment 11 • 7 years ago

Of course, go for it. I'll check in with you when I'm ready to help.

	Shing Lyu [:shinglyu] Reporter
	Comment 12 • 7 years 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

	Xidorn Quan [:xidorn] UTC+11
	Comment 13 • 7 years ago

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

	Shing Lyu [:shinglyu] Reporter
	Comment 16 • 7 years 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.

	Brad Werth [:bradwerth] Assignee
	Comment 17 • 7 years 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?

	Xidorn Quan [:xidorn] UTC+11
	Comment 18 • 7 years ago

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?

	Emilio Cobos Álvarez (:emilio)
	Comment 19 • 7 years ago

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

	Brad Werth [:bradwerth] Assignee
	Comment 20 • 7 years 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.

	Emilio Cobos Álvarez (:emilio)
	Comment 21 • 7 years ago

(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.

	Brad Werth [:bradwerth] Assignee
	Comment 22 • 7 years 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.

	Emilio Cobos Álvarez (:emilio)
	Comment 23 • 7 years ago

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.

	Brad Werth [:bradwerth] Assignee
	Comment 24 • 7 years 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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 25 • 7 years 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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 26 • 7 years 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.

	Brad Werth [:bradwerth] Assignee
	Comment 29 • 7 years 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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 30 • 7 years 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);
```

	Xidorn Quan [:xidorn] UTC+11
	Comment 31 • 7 years 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()`.

	Pulsebot
	Comment 34 • 7 years 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

	Carsten Book [:Tomcat]
	Comment 35 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a6deabb82919
https://hg.mozilla.org/mozilla-central/rev/d83cf405f54c