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

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: emilio, Assigned: boris)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

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

Updated

a year ago
Blocks: 1243581
Flags: needinfo?(hikezoe)
Flags: needinfo?(boris.chiou)
(Assignee)

Updated

a year ago
Assignee: nobody → boris.chiou
Flags: needinfo?(boris.chiou)
(Reporter)

Comment 1

a year ago
(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)
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 2

a year ago
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.
(Assignee)

Comment 3

a year ago
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)
(Reporter)

Comment 4

a year ago
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)
(Assignee)

Comment 5

a year ago
(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.)
(Assignee)

Comment 6

a year ago
(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 hidden (mozreview-request)
(Reporter)

Comment 8

a year ago
mozreview-review
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)
(Assignee)

Comment 9

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
(Reporter)

Comment 11

a year ago
mozreview-review
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)
(Reporter)

Comment 12

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 13

a year ago
mozreview-review-reply
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
(Reporter)

Comment 14

a year ago
mozreview-review
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+
(Assignee)

Comment 15

a year ago
(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.
(Assignee)

Comment 16

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

a year ago
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58c1de79d494
Use GeckoStyleContext in nsStyleTransformMatrix::ProcessTranslatePart. r=emilio
Priority: P1 → --

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/58c1de79d494
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1386848
(Assignee)

Updated

a year ago
Blocks: 1387667
(Assignee)

Updated

a year ago
Blocks: 1387668
(Assignee)

Updated

a year ago
No longer blocks: 1387668
You need to log in before you can comment on or make changes to this bug.