1367615 - Stylo: `inIDOMUtils.getSelectorCount` fails

Status: RESOLVED FIXED

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

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

DevTools expects `inIDOMUtils.getSelectorCount` to return a value, but currently it throws an error with Stylo.

This breaks tests like devtools/client/inspector/boxmodel/test/browser_boxmodel.js[1] and many others.

  Message: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [inIDOMUtils.getSelectorCount]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/css-logic.js :: CssLogic.getSelectors :: line 639"  data: no]
  Stack:
    CssLogic.getSelectors@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/css-logic.js:639:13
get selectors@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/css-logic.js:1003:21
processMatchedSelectors@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/css-logic.js:436:7
_findMatchedSelectors@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/css-logic.js:1220:5
get matchedSelectors@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/css-logic.js:1201:7
processMargins@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:821:11
getLayout@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:802:28
handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1082:19
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1760:15
receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:761:7
CssLogic.getSelectors@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/css-logic.js:639:13
get selectors@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/css-logic.js:1003:21
processMatchedSelectors@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/css-logic.js:436:7
_findMatchedSelectors@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/css-logic.js:1220:5
get matchedSelectors@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/css-logic.js:1201:7
processMargins@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:821:11
getLayout@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:802:28
handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1082:19
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1760:15
receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:761:7

[1]: https://treeherder.mozilla.org/logviewer.html#?job_id=101423408&repo=try&lineNumber=8527

Comment 1

[Bobby Holley (:bholley)]

jryans says this is the biggest blocker for devtools tests.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

You may also want to fix `getSelectorText` at the same time, it's called immediately after[1] `getSelectorCount`.

[1]: http://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/devtools/server/css-logic.js#636-645

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Bug 1367615 - Part 1: Move inDOMUtils style rules related functionality to BindingStyleRule.

Review commit: https://reviewboard.mozilla.org/r/146172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146172/

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Bug 1367615 - Part 2: Make ServoStyleRule implement nsICSSStyleRuleDOMWrapper.

Review commit: https://reviewboard.mozilla.org/r/146174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146174/

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Bug 1367615 - Part 3: Implement ServoStyleRule::GetSelectorCount and GetSelectorText.

Review commit: https://reviewboard.mozilla.org/r/146176/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146176/

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

The main issue was that ServoStyleRule was not implementing nsICSSStyleRuleDOMWrapper, so we were failing on this QI [1]. With the attached patches ServoStyleRule now implements nsICSSStyleRuleDOMWrapper and the implementation of the style rule related functionality is moved from inDOMUtils to each of the implementers of BindingStyleRule.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/inspector/inDOMUtils.cpp#322

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8874791 [details]
Bug 1367615 - Part 1: Move inDOMUtils style rules related functionality to BindingStyleRule.

https://reviewboard.mozilla.org/r/146172/#review150142

::: layout/style/BindingStyleRule.h:46
(Diff revision 1)
>  
>    // Likewise for this one.  We have to override our superclass, but don't
>    // really need to do anything in this method.
>    virtual bool IsCCLeaf() const override MOZ_MUST_OVERRIDE = 0;
>  
> +  virtual nsresult GetSelectorCount(uint32_t *aCount) = 0;

Given this one is infallible, perhaps it would make sense to just do:

```
virtual uint32_t GetSelectorCount()
```

wdyt?

::: layout/style/ServoStyleRule.cpp:266
(Diff revision 1)
> +nsresult
> +ServoStyleRule::SelectorMatchesElement(Element* aElement,
> +                                       uint32_t aSelectorIndex,
> +                                       const nsAString& aPseudo,
> +                                       bool* aMatches)
> +{

Please leave TODOS referencing bugs, or return unimplemented errors if  you don't end up implementing all these in this series.

::: layout/style/StyleRule.cpp:1473
(Diff revision 1)
> +                                  uint32_t aSelectorIndex,
> +                                  const nsAString& aPseudo,
> +                                  bool* aMatches)
> +{
> +  ErrorResult rv;
> +  nsCSSSelectorList* tail = GetSelectorAtIndex(aSelectorIndex, rv);

I'm assuming this is code move, so I'm not reviewing this too carefully, let me know if there's any bit that you think need some careful thought.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8874791 [details]
Bug 1367615 - Part 1: Move inDOMUtils style rules related functionality to BindingStyleRule.

https://reviewboard.mozilla.org/r/146172/#review150144

::: layout/style/ServoStyleRule.h:66
(Diff revision 1)
>                                                           css::Rule)
>    virtual bool IsCCLeaf() const final MOZ_MUST_OVERRIDE;
>    NS_DECL_NSIDOMCSSSTYLERULE
>  
> +  virtual nsresult GetSelectorCount(uint32_t *aCount) override;
> +  virtual nsresult GetSelectorText(uint32_t aSelectorIndex,

Oh, also, style guide says `virtual` here is redundant, so please fix it up, and perhaps `IsCCLeaf` too.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8874792 [details]
Bug 1367615 - Part 2: Make ServoStyleRule implement nsICSSStyleRuleDOMWrapper.

https://reviewboard.mozilla.org/r/146174/#review150146

Seems fine, but I'm not sure about all the cycle collector/interface map stuff, so I'd be more comfortable if heycam/xidorn reviewed this (because I'm not able to do all the reading on why what you're doing here is or isn't fine right now, sorry).

::: layout/style/ServoStyleRule.cpp:205
(Diff revision 1)
> +
> +NS_IMETHODIMP
> +ServoStyleRule::GetCSSStyleRule(BindingStyleRule **aResult)
> +{
> +  *aResult = this;
> +  NS_ADDREF(*aResult);

nit: You save a line if you do `NS_ADDREF(*aResult = this)`.

I think it's used elsewhere, but no big deal.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8874793 [details]
Bug 1367615 - Part 3: Implement ServoStyleRule::GetSelectorCount and GetSelectorText.

https://reviewboard.mozilla.org/r/146176/#review150150

::: layout/style/ServoStyleRule.cpp:281
(Diff revision 1)
>  ServoStyleRule::SelectorMatchesElement(Element* aElement,
>                                         uint32_t aSelectorIndex,
>                                         const nsAString& aPseudo,
>                                         bool* aMatches)
>  {
> +  // TODO Bug 1370502

Oh, ok, fair enough :)

::: servo/components/selectors/parser.rs:148
(Diff revision 1)
>               .map(SelectorList)
>      }
> +
> +    pub fn to_css_from_index<W>(&self, index: usize, dest: &mut W)
> +        -> fmt::Result where W: fmt::Write {
> +        let mut iter = self.0[index..].iter();

Gecko returns failure if `index` is out of bounds, probably we should do the same, or at least not panic and return an empty string, which you could accomplish with: `self.0.iter().skip(index)`.

I think both are acceptable, but please choose a behavior that doesn't make us crash if called with invalid values from JS :)

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8874791 [details]
Bug 1367615 - Part 1: Move inDOMUtils style rules related functionality to BindingStyleRule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146172/diff/1-2/

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8874792 [details]
Bug 1367615 - Part 2: Make ServoStyleRule implement nsICSSStyleRuleDOMWrapper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146174/diff/1-2/

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8874793 [details]
Bug 1367615 - Part 3: Implement ServoStyleRule::GetSelectorCount and GetSelectorText.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146176/diff/1-2/

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8874792 [details]
Bug 1367615 - Part 2: Make ServoStyleRule implement nsICSSStyleRuleDOMWrapper.

https://reviewboard.mozilla.org/r/146174/#review150446

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8874793 [details]
Bug 1367615 - Part 3: Implement ServoStyleRule::GetSelectorCount and GetSelectorText.

https://reviewboard.mozilla.org/r/146176/#review150580

with those, r=me

::: servo/components/selectors/parser.rs:146
(Diff revision 2)
>      where P: Parser<Impl=Impl> {
>          input.parse_comma_separated(|input| parse_selector(parser, input))
>               .map(SelectorList)
>      }
> +
> +    pub fn to_css_from_index<W>(&self, index: usize, dest: &mut W)

Let's call the `index` argument `from_index: usize`.

::: servo/components/selectors/parser.rs:149
(Diff revision 2)
>      }
> +
> +    pub fn to_css_from_index<W>(&self, index: usize, dest: &mut W)
> +        -> fmt::Result where W: fmt::Write {
> +        let mut iter = self.0.iter().skip(index);
> +        let first = iter.next()

You still can't call `expect` here, otherwise you'll crash.

You'll need to write it as:

```
let mut first = true;

for selector in iter {
    if !first {
        dest.write_str(", ")?;
    }
    selector.to_css(dest)?;
    first = false;
}
```

or:

```
let first = match iter.next() {
    Some(f) => f,
    None => return Ok(()),
};

first.to_css(dest)?;

for selector in iter {
    dest.write_str(", ")?;
    selector.to_css(dest)?;
}
```

Whatever you prefer.

::: servo/components/selectors/parser.rs:678
(Diff revision 2)
>      fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { self.to_css(f) }
>  }
>  
>  impl<Impl: SelectorImpl> ToCss for SelectorList<Impl> {
>      fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
> -        let mut iter = self.0.iter();
> +        self.to_css_from_index(0 /* index */, dest)

Let's use `/* from_index = */ 0`, to be consistent with other similar usages in Servo.

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8874791 [details]
Bug 1367615 - Part 1: Move inDOMUtils style rules related functionality to BindingStyleRule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146172/diff/2-3/

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8874792 [details]
Bug 1367615 - Part 2: Make ServoStyleRule implement nsICSSStyleRuleDOMWrapper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146174/diff/2-3/

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8874793 [details]
Bug 1367615 - Part 3: Implement ServoStyleRule::GetSelectorCount and GetSelectorText.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146176/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9526ccbaa1e7
Part 1: Move inDOMUtils style rules related functionality to BindingStyleRule. r=emilio
https://hg.mozilla.org/integration/autoland/rev/b1df5d005caf
Part 2: Make ServoStyleRule implement nsICSSStyleRuleDOMWrapper. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/763730b2f816
Part 3: Implement ServoStyleRule::GetSelectorCount and GetSelectorText. r=emilio

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9526ccbaa1e7
https://hg.mozilla.org/mozilla-central/rev/b1df5d005caf
https://hg.mozilla.org/mozilla-central/rev/763730b2f816