1042411 - "ASSERTION: It is assumed the initial operator is OPERATOR_OVER, when it is restored later"

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: It is assumed the initial operator is OPERATOR_OVER, when it is restored later: 'ctx->CurrentOperator() == gfxContext::OPERATOR_OVER', file layout/base/nsCSSRendering.cpp, line 2838

The testcase uses the "background-blend-mode" CSS property, which was added in bug 841601 and enabled in bug 970600.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Rik Cabanier]

Attachment: Fix that isolates moz-element drawing

I'm unsure how to write a test for this. Are asserts logged as part of testing?

Comment 3

[Rik Cabanier]

Attachment: Fix that isolates moz-element drawing + test

Comment 4

[Rik Cabanier]

try build: https://tbpl.mozilla.org/?tree=Try&rev=198a68ffcf9e

Comment 5

[Rik Cabanier]

Attachment: Fix that isolates moz-element drawing + test

removed debug code + extra tabs

Comment 6

[Rik Cabanier]

Attachment: Fix that isolates moz-element drawing + test

try build: https://tbpl.mozilla.org/?tree=Try&rev=a2cef9b425a2

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8462791 [details] [diff] [review]
Fix that isolates moz-element drawing + test

Review of attachment 8462791 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +4785,5 @@
>  
> +      gfxContext* ctx = aRenderingContext.ThebesContext();
> +      gfxContext::GraphicsOperator op = ctx->CurrentOperator();
> +      if (op != gfxContext::OPERATOR_OVER) {
> +        ctx->PushGroup(gfxContentType::COLOR);

This needs to be COLOR_ALPHA.

@@ +4795,5 @@
>                                 filter, aDest, aFill, aAnchor, aDirtyRect,
>                                 ConvertImageRendererToDrawFlags(mFlags));
> +
> +      if (op != gfxContext::OPERATOR_OVER) {
> +        ctx->PopGroupToSource();

Er, don't you need to call ctx->Paint() here?

Your test should have caught these bugs... The problem is probably that your -moz-element reference doesn't paint anything!

Comment 8

[Jesse Ruderman]

> I'm unsure how to write a test for this. Are asserts logged as part of testing?

Yes, most of our test suites (including reftest) count asserts and fail unless the test is annotated as expecting that number of asserts.  I see that you made a reftest pair to ensure the correct behavior, which is even better.

Comment 9

[Rik Cabanier]

Attachment: Fix that isolates moz-element drawing + test

You're right. The "paint" call was needed.
I updated the code and the test so it draws something.

try build: https://tbpl.mozilla.org/?tree=Try&rev=b3fc9c6293dd

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8463213 [details] [diff] [review]
Fix that isolates moz-element drawing + test

Review of attachment 8463213 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +4785,5 @@
>  
> +      gfxContext* ctx = aRenderingContext.ThebesContext();
> +      gfxContext::GraphicsOperator op = ctx->CurrentOperator();
> +      if (op != gfxContext::OPERATOR_OVER) {
> +        ctx->PushGroup(gfxContentType::COLOR);

This should be COLOR_ALPHA. Your test should test something that isn't fully opaque.

Comment 11

[Rik Cabanier]

Attachment: Fix that isolates moz-element drawing + test

I was wondering what that parameter did. I couldn't find documentation so picked the API that sounded right.

Update code + made test transparent + verified that the test was broken without the fix
try run: https://tbpl.mozilla.org/?tree=Try&rev=2ccbf0faea91

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8463506 [details] [diff] [review]
Fix that isolates moz-element drawing + test

Review of attachment 8463506 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +4785,5 @@
>  
> +      gfxContext* ctx = aRenderingContext.ThebesContext();
> +      gfxContext::GraphicsOperator op = ctx->CurrentOperator();
> +      if (op != gfxContext::OPERATOR_OVER) {
> +        ctx->PushGroup(gfxContentType::ALPHA);

COLOR_ALPHA. I mean it :-).

The temporary buffer needs both color channels and an alpha channel. That's what COLOR_ALPHA means. I'm not sure how this passes your test...

Comment 13

[Rik Cabanier]

Attachment: mozelementblend.patch

argh, sorry about that. Updated patch is attached.

try run: https://tbpl.mozilla.org/?tree=Try&rev=30aaecebdb73

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79d9d8c334cb

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/79d9d8c334cb