1030604 - SVG gradient opacity mask renders DIVElement blank when applied in some cases

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Demo video @ YouTube (please turn on HD)

I was doing bug 1023500 and hit this bug. Attached is the demo video using B2G "template" app in https://github.com/mnjul/gaia/tree/demo_svg_gradient_opacity_mask_div_empty (only the tip commit is relevant)

Synopsis:
when a SVG gradient mask is added onto a DIV element, it can make DIV element graphically blank under some scenario.

STR with Flame (B2G reference phone): (please refer to demo video)
1. On phone, start the "template" app provided in my github repository above
2. On desktop, start App Manager on Nightly and Debug the "template" app
3. On phone, in "template" app, click "Add an item" exactly 5 times.
3a. On the 5th click, you see that a gradient opacity mask is applied to the list container, and the container fades out to transparency at its bottom.
4. (Optional) Click "Add an item" 2 more times to make sure the mask renders things perfectly.
5. Click "Clear all items". The list should be cleared.
5a. At this moment, the mask is removed from the list container by my code.
6. Click "Add an item" exactly 5 times. At this moment, the gradient opacity mask is applied by the codes again.

Expected:
On step 6, on the 5th click, the gradient mask renders correctly and the list container fades out to transparency at its bottom.

Actual:
On step 6, on the 5th click, The list container becomes graphically blank.


Additional Info:
At step 6, when the container is blank, if you use App Manager's inspector to inspect the container element, it will suddenly re-appear. Also at step 6, when the container is blank, if you use click "Add an item" one more time, the container will suddenly re-appear too. So I think this is some graphics/rendering engine issue in gecko.

Environment:
latest gecko master and latest gaia master

This bug is also applicable to Nightly desktop browser. More info in some subsequent comment.

Comment 1

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: template_desktop.zip

This issue also happens on desktop Nightly (I am on Ubuntu 14.04 64-bit, and the nightly is 2014-06-25 33.0a1).

Check the attached file for a demo webpage and associated styles/codes. They contain almost the same codes as my demo B2G "template" app.

STR is almost the same, simplified as follows:
1. Click "Add an item" exactly 5 times.
2. Click "Clear all items".
3. Click "Add an item" exactly 5 times.

Expected:
On step 3, on the 5th click, the gradient mask renders correctly and the list container fades out to transparency at its bottom.

Actual:
On step 3, on the 5th click, The list container becomes graphically blank.

Comment 2

[John Lu [:mnjul][Please NI or I'll miss]]

Hi Jerry,

I was told that you might be able to delegate this to somebody properly responsible for this bug. Could you check this out? Thanks.

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Seth,
Could you give some comment about this issue? I'm not familiar with the svg rendering path.
Thanks.

Comment 4

[howie [:howie]]

Hi David and Daniel, could you provide assistance here if you are the right person? Thank you very much! ni Ivan since the component is set to Graphics.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to John Lu [:mnjul] [Out-of-Office 7/11~7/20] [MoCoTPE] from comment #1)
> Created attachment 8446359 [details]
[...]
> STR is almost the same, simplified as follows:
> 1. Click "Add an item" exactly 5 times.

After this step (the first time I see some transparency, on the bottom-most item), I get two instances of this assertion, in my debug build:
###!!! ASSERTION: How did we getting here, then?: 'aFrame->GetParent()->StyleContext()->GetPseudo() == nsCSSAnonBoxes::mozAnonymousBlock', file layout/svg/nsSVGIntegrationUtils.cpp, line 111
http://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGIntegrationUtils.cpp?rev=fb2ab91ddd34#109

It seems likely that any brokenness after that point is related to that.

(Bug 842114 is also filed on a testcase that triggers this assertion. May be related.)

Comment 6

[howie [:howie]]

This is actually a blocker, without this being fixed, notifications disappear / appear when we have more than 4. Another demo: https://www.youtube.com/watch?v=hZzvkitPjXw&feature=youtu.be

So Daniel, Greg, could we try the patch in bug 842114 to see if it solves this issue? Thank you!

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Did this go through full triage of the release drivers to get 2.0+, or Howie, did you just assume this to be the case? I only saw the "use the template app and put together a test case", I don't see a shipping or a partner app getting in trouble.  Is there another test case?

Comment 8

[Robert Longson [:longsonr]]

(In reply to howie [:howie] from comment #6)
> 
> So Daniel, Greg, could we try the patch in bug 842114 to see if it solves
> this issue? Thank you!

The patch in that build is not a fix, it's just diagnostics trying to provide more details when something goes wrong.

Comment 9

[Robert Longson [:longsonr]]

s/build/bug/

Comment 10

[Greg Weng [:snowmantw][:gweng][:λ]]

(In reply to Milan Sreckovic [:milan] from comment #7)
> Did this go through full triage of the release drivers to get 2.0+, or
> Howie, did you just assume this to be the case? I only saw the "use the
> template app and put together a test case", I don't see a shipping or a
> partner app getting in trouble.  Is there another test case?

Hi, while I understand a full triage is necessary for blocker+, I think Howie mark it because Bug 1023500 is a blocker, which is mainly for notification on LockScreen, and this bug would make it totally broken even with the visual update patch. This video would show after the Bug 1023500 got patched, the notification would still be broken since this bug is still there:

https://www.youtube.com/watch?v=ON_qVIkvCW4&feature=youtu.be

(Sorry for the annoying pan moving) 

You can see the notifications suddenly disappear as the first comment describing.
The symptom would disappear after rebooting. It would appear again if the screen got black out.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Here's a somewhat-reduced standalone testcase.

(I've become less-convinced that the assertion is related to the root problem here, too, because while working on this testcase, I had some dead-ends where the assertion did fire, but everything painted correctly.)

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (automated; says "PASS" if passed)

Here's an automated testcase, which starts out with a masked child, removes the mask & child, and then adds them back (with some carefully-placed reflows via "offsetHeight").

This one doesn't trigger the assertion from comment 5, so that assertion is indeed unrelated & should no longer be considered here. (sorry for that red herring)

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So one thing that seems suspicious:

ComputeEffectsRect in nsFrame.cpp (a helper function for nsIFrame::FinishAndStoreOverflow) stores a PreEffectsBBoxProperty and then adjusts the overflow rect for the effects.  Dynamic changes to the mask property don't trigger UpdateOverflow.  It seems to me that all properties for which nsSVGIntegrationUtils::UsingEffectsForFrame returns true should return nsChangeHint_UpdateOverflow.  Currently we do this for filters but not the others.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes, adding an UpdateOverflow hint fixes it; I'll clean up the patch.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> Created attachment 8456312 [details]
> testcase 2 (automated; says "PASS" if passed)
> 
> Here's an automated testcase, which starts out with a masked child, removes
> the mask & child, and then adds them back (with some carefully-placed
> reflows via "offsetHeight").

Thanks; this testcase was useful.

> This one doesn't trigger the assertion from comment 5, so that assertion is
> indeed unrelated & should no longer be considered here. (sorry for that red
> herring)

I think the assertion was somewhat related; I think the assertion and the bug are both symptoms of the same problem, which is failure to keep the PreEffectsBBoxProperty up-to-date.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Use nsChangeHint_UpdateOverflow for clip-path and mask changes so we update the PreEffectsBBoxProperty and run the effect on the correct area

The testcase is a slight simplification of dholbert's testcase 2
(attachment 8456312 [details]) in the bug.  It fails in the reftest harness
without the patch, and passes in the reftest harness with the patch.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://tbpl.mozilla.org/?tree=Try&rev=daa883071450

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #15)
> I think the assertion was somewhat related; I think the assertion and the
> bug are both symptoms of the same problem, which is failure to keep the
> PreEffectsBBoxProperty up-to-date.

For the record: I can confirm that the attached patch fixes the assertion as well as the testcase on the attached "testcase 1" & "template_desktop.zip" (and it fixes the rendering bug in all testcases here), but it does not fix the assertion on bug 842114's testcase. (attachment 714873 [details])

So, that bug should remain open.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dd8407fd66b

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8dd8407fd66b

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/88a84442b216