1384656 - stylo: We use nsStyleTransformMatrix, which can enter in nsRuleNode and assume that we use a Gecko style context.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

Right now we assert there and crash, but that really should hold.

The case is when CalcLength or SpecifiedCalcToComputedCalc try to handle rem units. I think it shouldn't be too hard to find a test-case that crashes.

I found this when investigating bug 1384542.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

(The title is wrong, I meant nsStyleTransformMatrix).

Thanks for taking this Boris!

Comment 2

[Boris Chiou [:boris]]

After checking ProcessTranslatePart() and ToComptuedValue of SpecifiedOperation::Translate, it seems that it is impossible to let an em/rem arrive this function (sorry, my fault in irc), because we convert an em/rem into pixel value (i.e. Au) during converting the specified length value into computed length value.

Therefore, only nsRuleNode::SpecifiedCalcToComputedCalc() will be called in ProcessTranslatePart(). However, SpecifiedCalcToComputedCalc() may call CalcLength(), and I am trying to understand why we hit this assertion because it seems there is no rem arrive here.

Comment 3

[Boris Chiou [:boris]]

Hi Emilio,

Sorry, I need more info about how to hit the assertion. I was wrong in the irc because we always convert rem into Au in to_computed_value() for Translate, and so it's impossible to let "rem" arrive in nsStyleTransform. (Even in Calc type, we shouldn't have any rem in LengthPercentPairCalcOps.) Therefore, the assertion you hit is the case I didn't notice.

(We may add some assertions in ProcessTranslatePart() to make sure we only have pixel/number/percent/calc types if Servo backend is true.)

Thanks.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yeah, I _didn't_ hit the assertion, but I was converting that code to GeckoStyleContext, and we can definitely arrive there with a normal Servo style context.

That being said, if we don't have relative units, we can pass null as the style context I think, or add assertions.

Comment 5

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> That being said, if we don't have relative units, we can pass null as the
> style context I think, or add assertions.

Yes. We can just pass null style context for stylo because we don't really need it. (And maybe revise the assertion in CalcLength(), or avoid calling it because computed::LengthOrPercentage already resolve the calc operators and only contains an Au and a percentage.)

Comment 6

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #5)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> > That being said, if we don't have relative units, we can pass null as the
> > style context I think, or add assertions.
> 
> Yes. We can just pass null style context for stylo because we don't really
> need it. (And maybe revise the assertion in CalcLength(), or avoid calling
> it because computed::LengthOrPercentage already resolve the calc operators
> and only contains an Au and a percentage.)

s/LengthOrPercentage/CalcLengthOrPercentage/

Comment 7

[Boris Chiou [:boris]]

Attachment: Bug 1384656 - Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart.

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8891934 [details]
Bug 1384656 - Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart.

https://reviewboard.mozilla.org/r/162966/#review168284

::: layout/style/nsStyleTransformMatrix.cpp:163
(Diff revision 1)
>      // Raw numbers are treated as being pixels.
>      //
>      // Don't convert to aValue to AppUnits here to avoid precision issues.
>      return aValue.GetFloatValue();
>    } else if (aValue.IsCalcUnit()) {
> +    if (aContext && aContext->IsGecko()) {

Why is this needed? This function is supposed to handle null just fine:

http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/layout/style/nsStyleTransformMatrix.cpp#835

Can we just make the argument a `GeckoStyleContext*`, propagating it as needed?

Comment 9

[Boris Chiou [:boris]]

Comment on attachment 8891934 [details]
Bug 1384656 - Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart.

https://reviewboard.mozilla.org/r/162966/#review168284

> Why is this needed? This function is supposed to handle null just fine:
> 
> http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/layout/style/nsStyleTransformMatrix.cpp#835
> 
> Can we just make the argument a `GeckoStyleContext*`, propagating it as needed?

Thanks for the review. I added this condition because null nsStyleContext is fine only if we make sure the unit of nsCSSValue is pixel, number, or percentage (because we shouldn't pass null style oontext to nsRuleNode::XXX).

I will change the type of ```nsStyleContext*``` to ```GeckoStyleContext*```. In this if-branch (i.e. if(aValue.IsCalcUnit())), if ```GeckoStyleContext*``` is nullptr and nsCSSValue is eCSSUnit_Calc (i.e. Servo backend), we call GetCalcValue directly for stylo; otherwise, we call nsRuleNode::SpecifiedCalcToComputedCalc() for Gecko backend.

Comment 10

[Boris Chiou [:boris]]

Comment on attachment 8891934 [details]
Bug 1384656 - Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart.

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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8891934 [details]
Bug 1384656 - Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart.

https://reviewboard.mozilla.org/r/162966/#review168612

::: layout/painting/nsDisplayList.cpp:183
(Diff revision 2)
>  {
>    if (aList->mValue.GetUnit() == eCSSUnit_None) {
>      return;
>    }
>  
> +  GeckoStyleContext* geckoContext = aContext && aContext->IsGecko()

`aContext ? aContext->GetAsGecko() : nullptr`.

::: layout/style/nsStyleTransformMatrix.cpp:137
(Diff revision 2)
>    mWidth = aDimensions.width;
>    mHeight = aDimensions.height;
>    mIsCached = true;
>  }
>  
>  float

I think this should be `static`, maybe fix whyle yo're here?

::: layout/style/nsStyleTransformMatrix.cpp:163
(Diff revision 2)
>      // Raw numbers are treated as being pixels.
>      //
>      // Don't convert to aValue to AppUnits here to avoid precision issues.
>      return aValue.GetFloatValue();
>    } else if (aValue.IsCalcUnit()) {
> +    if (aValue.GetUnit() == eCSSUnit_Calc) {

Again, do we need it? SpecifiedCalcToComputedCalc doesn't seem to use `mContext` in any case we care about... If we don't, please let's just remove it.

::: layout/style/nsStyleTransformMatrix.cpp:999
(Diff revision 2)
>                 TransformReferenceBox& aRefBox,
>                 float aAppUnitsPerMatrixUnit,
>                 bool* aContains3dTransform)
>  {
>    Matrix4x4 result;
> +  GeckoStyleContext* geckoContext = aContext && aContext->IsGecko()

Pretty sure this can be: `aContext ? aContext->GetAsGecko() : nullptr`.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8891934 [details]
Bug 1384656 - Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart.

https://reviewboard.mozilla.org/r/162966/#review168612

> `aContext ? aContext->GetAsGecko() : nullptr`.

Also, maybe name thevariable `contextIfGecko`? though maybe not a big deal.

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8891934 [details]
Bug 1384656 - Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart.

https://reviewboard.mozilla.org/r/162966/#review168612

> Also, maybe name thevariable `contextIfGecko`? though maybe not a big deal.

I see. I will update this. Thanks.

> Again, do we need it? SpecifiedCalcToComputedCalc doesn't seem to use `mContext` in any case we care about... If we don't, please let's just remove it.

I think it's better to add this. SpecifiedCalcToComputedCalc() uses LengthPercentPairCalcOps and ComputeCalc(xxx, xxx, op) [1], and it finally calls LengthPercentPairCalcOps::ComputeLeafValue(), which will call CalcLength() [2], and then assert if we use null nsStyleContext [3].

[1] http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/layout/style/nsRuleNode.cpp#844
[2] http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/layout/style/nsRuleNode.cpp#766
[3] http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/layout/style/nsRuleNode.cpp#714

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8891934 [details]
Bug 1384656 - Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart.

https://reviewboard.mozilla.org/r/162966/#review168640

r=me, though a few of "shared list means backend type" checks make me feel uncomfortable... If you think it's better to pass the context a bit more down instead please go for it.

Comment 15

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> r=me, though a few of "shared list means backend type" checks make me feel
> uncomfortable...

I don't like this way, either. However, I don't have a better idea to check the backend for now if we don't pass extra info. I will file a follow-up bug for this.

Comment 16

[Boris Chiou [:boris]]

Comment on attachment 8891934 [details]
Bug 1384656 - Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart.

https://reviewboard.mozilla.org/r/162966/#review168612

> I think this should be `static`, maybe fix whyle yo're here?

Oh. Unfortunately, nsStyleTransformMatris is just a namespace, not a class, and ProcessTranslatePart() is also needed in nsDisplayList.cpp (i.e. so it is a public API).

Comment 17

[Boris Chiou [:boris]]

Comment on attachment 8891934 [details]
Bug 1384656 - Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart.

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

Comment 18

[Boris Chiou [:boris]]

Comment on attachment 8891934 [details]
Bug 1384656 - Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart.

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58c1de79d494
Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart. r=emilio

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/58c1de79d494