Closed Bug 1384656 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: emilio, Assigned: boris)

References

Details

Attachments

(1 file)

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.
Blocks: stylo
Flags: needinfo?(hikezoe)
Flags: needinfo?(boris.chiou)
Assignee: nobody → boris.chiou
Flags: needinfo?(boris.chiou)
(The title is wrong, I meant nsStyleTransformMatrix).

Thanks for taking this Boris!
Summary: stylo: We use nsStyleAnimationValue, which can enter in nsRuleNode and assume that we use a Gecko style context. → stylo: We use nsStyleTransformMatrix, which can enter in nsRuleNode and assume that we use a Gecko style context.
Flags: needinfo?(hikezoe)
Status: NEW → ASSIGNED
Priority: -- → P1
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.
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.
Flags: needinfo?(emilio+bugs)
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.
Flags: needinfo?(emilio+bugs)
(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.)
(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 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?
Attachment #8891934 - Flags: review?(emilio+bugs)
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 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`.
Attachment #8891934 - Flags: review?(emilio+bugs)
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 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 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.
Attachment #8891934 - Flags: review+
(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 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).
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58c1de79d494
Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart. r=emilio
Priority: P1 → --
https://hg.mozilla.org/mozilla-central/rev/58c1de79d494
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1386848
Blocks: 1387667
Blocks: 1387668
No longer blocks: 1387668
You need to log in before you can comment on or make changes to this bug.