Closed Bug 894594 Opened 6 years ago Closed 5 years ago

Visual glitch on about:newtab demo-rewrite when applying/removing "box-shadow" with :hover, in debug builds, with nested CSS3 flexboxes (due to reflow being interrupted)

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: johan.charlez, Assigned: dholbert)

References

Details

Attachments

(2 files)

Attached file Testcase 1
I'm seeing visual glitches when hovering the grid in the testcase on a local mozila-central build.

The grid is built using CSS3 flexbox.

My .mozconfig file:
=====================
  mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../obj
  mk_add_options MOZ_MAKE_FLAGS="-s --no-print-directory -j6"
  ac_add_options --disable-debug-symbols
=====================

Building using start-msvc10.
Forgot to mention that I can't reproduce this on Nightly.
Neither Johan nor I can reproduce in a nightly build, but we see it on Linux and Windows local builds. Matt Brubeck doesn't see it on Windows in his local build. This is spooky.
I can reproduce on my Linux debug build.
This testcase has several layers of nested flex containers, so the measuring reflows can blow up a bit and take up some time.  It also has a "box-shadow" hover state, which makes us redo a bunch of work (including a reflow, at least, apparently).

In debug builds, I suspect the box-shadow-triggered reflow might be taking too long, and we interrupt layout partway through (to paint and respond to user events), and that leaves us painting at an intermediate state with the results of a "measuring" reflow being painted (for flex items), instead of the results of a real reflow. (During CSS3 flexbox layout, we may do some fake "measuring-only" reflows, to measure the auto height of a flex item (if we need it), before we resolve its final height.)

Or something like that.
Summary: Visual glitch on hover (flexbox) → Visual glitch when applying/removing "box-shadow" with :hover, in debug builds, with nested CSS3 flexboxes
Version: unspecified → Trunk
(In reply to Daniel Holbert [:dholbert] from comment #4)
> Created attachment 776720 [details]
> testcase 2 (somewhat reduced)

I can't reproduce the problem with this testcase.

I did however notice something interesting. With testcase 1 the problem goes away when my laptop is plugged in (i.e. not running on battery power).

I am seeing the problem with hardware acceleration off as well as on.
(In reply to Daniel Holbert [:dholbert] from comment #5)
> This testcase has several layers of nested flex containers, so the measuring
> reflows can blow up a bit and take up some time.  It also has a "box-shadow"
> hover state, which makes us redo a bunch of work (including a reflow, at
> least, apparently).
> 
> In debug builds, I suspect the box-shadow-triggered reflow might be taking
> too long, and we interrupt layout partway through (to paint and respond to
> user events), and that leaves us painting at an intermediate state with the
> results of a "measuring" reflow being painted (for flex items), instead of
> the results of a real reflow. (During CSS3 flexbox layout, we may do some
> fake "measuring-only" reflows, to measure the auto height of a flex item (if
> we need it), before we resolve its final height.)
> 
> Or something like that.

Unless I'm entirely mistaken builds are not debug builds by default?

And I'm not building with "--enable-debug".
Correct, builds aren't debug by default.

I think this is just perf-dependent, so the exact conditions that trigger it are fuzzy.  I'm not sure why you can repro in a local opt build but not in a nightly, but I'm not going to worry about that too much.
(In reply to Johan Charlez from comment #6)
> > testcase 2 (somewhat reduced)
> 
> I can't reproduce the problem with this testcase.

I can't either now. (though I still can reproduce with testcase 1, in debug trunk builds)
(In reply to Daniel Holbert [:dholbert] from comment #5)
> In debug builds, I suspect the box-shadow-triggered reflow might be taking
> too long, and we interrupt layout partway through

BTW: I verified that this is true. The problem goes away if I add
  nsPresContext::InterruptPreventer preventer(aPresContext);
somewhere near the beginning of nsFlexContainer::Reflow.

That would work as a fix, but it might be too heavy-handed. Ideally it'd be nice if we could prevent interrupts in a few isolated places (e.g. around the trial reflows), or something like that, rather than for all of nsFlexContainerFrame reflow.
Summary: Visual glitch when applying/removing "box-shadow" with :hover, in debug builds, with nested CSS3 flexboxes → Visual glitch on about:home demo-rewrite when applying/removing "box-shadow" with :hover, in debug builds, with nested CSS3 flexboxes (due to reflow being interrupted)
Summary: Visual glitch on about:home demo-rewrite when applying/removing "box-shadow" with :hover, in debug builds, with nested CSS3 flexboxes (due to reflow being interrupted) → Visual glitch on about:newtab demo-rewrite when applying/removing "box-shadow" with :hover, in debug builds, with nested CSS3 flexboxes (due to reflow being interrupted)
Good news -- the issues with Testcase 1 (which are really from reflow taking too long & getting interrupted, per comment 10) seem to be fixed by bug 1015474's patches.

This is likely because those patches save us from having to double-reflow a flex item in some cases, as described in bug 946167 comment 21.

Adding dependency.
Depends on: 1015474
Following up on comment 11 -- I've verified that I can't reproduce this anymore, in an up-to-date debug build (with bug 1015474's patches).

So, I'm resolving this as FIXED by bug 1015474. Hooray!
Assignee: nobody → dholbert
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.