1532444 - Dynamic change to marker style causes it to continuously repaint forever

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:
0. Make sure you have web render disabled and paint flashing enabled:

  gfx.webrender.all = false
  nglayout.debug.paint_flashing = true

1. View attached testcase.

EXPECTED RESULTS:
One second after loading, when the text becomes bold, its background-color should flash once (indicating a single repaint).

ACTUAL RESULTS:
When the text becomes bold, the background starts flashing continuously and never stops. It looks like we get into a never-ending "invalidate/repaint/invalidate/etc." loop.

Comment 1

[Robert Longson [:longsonr]]

Is the <text> element required? If so did this work properly before Cam's <text> element rewrite?

Comment 2

[Daniel Holbert [:dholbert]]

Hmm, good thoughts!

• the text element does indeed seem to be required
• I can't reproduce flashing in a build from 2013-01-01

So this is a regression from some point (perhaps Cam's text rewrite).

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (comparing rect vs text)

Comment 4

[Daniel Holbert [:dholbert]]

Also: in a build from 2015-07-11, the text continuously repaints even before I've made a dynamic style change.

Comment 5

[Daniel Holbert [:dholbert]]

Here's the fix range, for where we went from immediate continuous-repaints (comment 4) to only repainting once a dynamic style change has been made (the current behavior):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=30ea2905130e85f9e1d8d56fa3097901eec6514b&tochange=67cd1ee26f2661fa5efe3d952485ab3c89af4271

Comment 6

[Robert Longson [:longsonr]]

For text we'll end up in SVGTextFrame::ScheduleReflowSVGNonDisplayText somehow we're triggering that again and again I assume.

Comment 7

[Daniel Holbert [:dholbert]]

The original regression range here (going from "no continuous paint flashing" to "immediate & continuous flashing" per comment 4, for text) was:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2114ef80f6ae&tochange=b62ccf3228ba

Looks like probably a regression from bug 1090936.

Comment 8

[Robert Longson [:longsonr]]

I suspect the SVGTextFrame::ReflowSVGNonDisplayText change was wrong and should be reverted then.

Comment 9

[Robert Longson [:longsonr]]

Attachment: strawman.txt

Comment 10

[Robert Longson [:longsonr]]

Attachment: Bug 1532444 - Dynamic change to marker with text content's style causes it to continuously repaint forever. r=dholbert

Revert the invalid part of bug 1090936 as SVGTextFrame::ReflowSVGNonDisplayText is already asynchronous
as callers invoke it via SVGTextFrame::ScheduleReflowSVGNonDisplayText

Comment 11

[Robert Longson [:longsonr]]

I checked that we get one update on the style change and no continuous paint flashing now.

Comment 12

[Robert Longson [:longsonr]]

Thanks for the help figuring it out Daniel, comment 2 and the bug causing the regression really pinpointed the error.

Comment 13

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dcd37bde6fb13b2e12e01fd55466810ac888a7f

Comment 14

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c7a617b97a65c42f5df0ba8900605384d552a8a

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b4af1ecd78d
Go back to doing synchronous invalidation in ReflowSVGNonDisplayText as its invoked by ScheduleReflowSVGNonDisplayText so is already asynchronous r=dholbert

Comment 16

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/6b4af1ecd78d

Comment 17

[Emilio Cobos Álvarez (:emilio)]

It would've been nice to land a test for this...

Comment 18

[Robert Longson [:longsonr]]

Any ideas how one would write such a test?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

We should be able to use nsIDOMWindowUtils.checkAndClearPaintedState() to ensure that we don't paint the frame in two consecutive frames without any change.