491537 - Google reader items in list view no longer expand when clicked on

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Bill Gianopoulos [:WG9s]]

Using this build:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090505 Firefox/3.6a1pre ID:20090505044648 build from changeset http://hg.mozilla.org/mozilla-central/rev/a08d1947ec5a

In Google Reader, when I have the List view selected for the main viewing pane clicking on the title of an article in the list is supposed to expand just that one item.

Since the check-in for bug 67752, in order to get the expanded view to display I need to resize the window to force a reflow.  If I have the expanded view selected for the Main viewing pane, then everything works as it should.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Aha!  I tend to use the expanded view.  I'll try this with the list view.

Comment 2

[Bill Gianopoulos [:WG9s]]

OK.

It appears that this is also aggravated by the fact that my Internet
connection is being a bit slow today.  If I try to expand the article before
the list finishes displaying (as indicated by the more than xx items at the
bottom of the screen), I get the issue.  But that is not the only thing. I have
also noticed that in some cases the list never seems to finish displaying and
stops after displaying around 16 items.

This is all easier to reproduce if you have it displaying all items rather than
just unread items so that the list has more items.  If it does not mess up on
the first folder I look at, then it usually will on the second or third.

Comment 3

[Bill Gianopoulos [:WG9s]]

Another issue, probably related and easier to reproduce, is that vertically resizing the window does not result in proper repainting of the article list pane.

The article list should resize to fit the window, ending with the bar containing the "Previous item" and "Next item" buttons at the bottom of the window.

Comment 4

[Bill Gianopoulos [:WG9s]]

When I get the original issue, it asserts with:

###!!! ASSERTION: Why is the target not dirty?: 'NS_SUBTREE_DIRTY(target)', file /home/wag/mozilla/mozilla2/layout/base/nsPresShell.cpp, line 7023

When I am resizing, I also get the assertion:

###!!! ASSERTION: cannot call GetUsedBorder on a dirty frame not currently being reflowed: 'nsLayoutUtils::sDisableGetUsedXAssertions || !NS_SUBTREE_DIRTY(this) || (GetStateBits() & NS_FRAME_IN_REFLOW)', file /home/wag/mozilla/mozilla2/layout/generic/nsFrame.cpp, line 622

Comment 5

[Bill Gianopoulos [:WG9s]]

I neglected to mention in comment #4 that this was with a Linux debug build.

Comment 6

[Bill Gianopoulos [:WG9s]]

Attachment: Workaround

It seems that making the Flush_Layout in PresShell::WillPaint non-interruptible is sufficient to avoid this issue.  It is not just that the flush is interrupted that causes the issue.  If you comment out the entire line of code the issue remains.

As a further test to isolate the issue, I tried pulling the source from before the interruptible relow landing and commenting out the flush in WillPaint.  That did NOT trigger the issue.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> ###!!! ASSERTION: Why is the target not dirty?: 'NS_SUBTREE_DIRTY(target)',

Oh, that would totally do it, of course.  I wonder how we end up in that state....  I'll dig.

Comment 8

[Boris Zbarsky [:bzbarsky]]

For some reason we end up with an abs-pos child of the canvas which has NS_FRAME_HAS_DIRTY_CHILDREN set on it when we get to looking at it in MarkFramesDirtyToRoot (to which it's the argument).  But its ancestors have no dirty bits set, which means we don't ever mark the CanvasFrame and ViewportFrame as needing reflow, and then don't reflow any of this stuff.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ok, the problem is that nsAbsoluteContainingBlock::Reflow marks kids after an interrupt (including possible the kids that the interrupt happened in) with dirty bits.  But it doesn't make any calls to FrameNeedsToContinueReflow on the delegating frame.  There used to be a MarkPathToFrameDirty(aDelegatingFrame) call there, but it got taken out per review comments.  I'd just forgotten why I had needed it.  Columns have a similar issue.  I'll patch tomorrow.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Neeti, do you want to add a unit test for this using your test infrastructure?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 12

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

Why isn't the interrupt that interrupted within the reflow of the kids sufficient for marking things dirty?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Because we do the dirty marking after reflow has completed.  So without the diff attached the sequence of events in the simplest case is:

1)  canvas reflows block abs pos child which interrupts.
2)  said block child is added to the "mark dirty" list.
3)  reflow of block child completes and all dirty bits are removed.
4)  absolute containing block adds a dirty bit back to the block child due to
    that block at the end of the reflow loop.
5)  We finish up reflow, go to mark ancestors of the block dirty, see that it's
    already dirty, and bail out.  We never add dirty bits to the canvas.

Since in some cases we need to add NS_FRAME_IS_DIRTY, not just HAS_DIRTY_CHILDREN, getting rid of step 4 is hard.  Getting rid of the dirty check in step 5 leads to O(N^2) behavior in depth of block tree for the "add dirty bits" step.  So the patch just makes sure to put the canvas (in the above-described case) into the list of things that need to have ancestors marked dirty.

Comment 14

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

Comment on attachment 376452 [details] [diff] [review]
Fix

OK, but maybe the comments at both locations could be improved?  I didn't understand the issue until your explanation in the previous comment.

r+sr=dbaron

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/42a3db8c8837 with what I hope are improved comments.

Comment 16

[Boris Zbarsky [:bzbarsky]]

This landed before the 1.9.2 branchpoint.

Comment 17

[Jonathan Griffin (:jgriffin)]

Fix verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b1pre) Gecko/20090929 Namoroka/3.6b1pre (.NET CLR 3.5.30729)