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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Johan C]

Attachment: 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.

Comment 1

[Johan C]

Forgot to mention that I can't reproduce this on Nightly.

Comment 2

[Josh Matthews [:jdm]]

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.

Comment 3

[Daniel Holbert [:dholbert]]

I can reproduce on my Linux debug build.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (somewhat reduced)

Comment 5

[Daniel Holbert [:dholbert]]

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.

Comment 6

[Johan C]

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

Comment 7

[Johan C]

(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".

Comment 8

[Daniel Holbert [:dholbert]]

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.

Comment 9

[Daniel Holbert [:dholbert]]

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

Comment 10

[Daniel Holbert [:dholbert]]

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

Comment 11

[Daniel Holbert [:dholbert]]

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.

Comment 12

[Daniel Holbert [:dholbert]]

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!