Dynamic change to marker style causes it to continuously repaint forever

RESOLVED FIXED in Firefox 67

Status

()

defect
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: dholbert, Assigned: longsonr)

Tracking

({regression})

Trunk
mozilla67
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

Details

(Whiteboard: [qf:p3:resource])

Attachments

(3 attachments, 1 obsolete attachment)

Posted file 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.

Whiteboard: [qf] → [qf:p3:resource]

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

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

Keywords: regression

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

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

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

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.

Depends on: 1090936

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

Posted patch strawman.txt (obsolete) — Splinter Review

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

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

Attachment #9048690 - Attachment is obsolete: true

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

Assignee: nobody → longsonr
Status: NEW → ASSIGNED
Pushed by longsonr@gmail.com:
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
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

Any ideas how one would write such a test?

Flags: needinfo?(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.

Flags: needinfo?(emilio)
Blocks: 1090936
No longer depends on: 1090936
Flags: in-testsuite?
QA Whiteboard: [qa-67b-p2]
You need to log in before you can comment on or make changes to this bug.