Closed Bug 491537 Opened 11 years ago Closed 11 years ago

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


(Core :: Layout, defect, P1)

Windows XP



Tracking Status
status1.9.2 --- beta1-fixed


(Reporter: wgianopoulos, Assigned: bzbarsky)




(Keywords: regression)


(2 files)

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

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.
Aha!  I tend to use the expanded view.  I'll try this with the list view.
Flags: blocking1.9.2+
Priority: -- → P1

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.
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.
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
I neglected to mention in comment #4 that this was with a Linux debug build.
Attached patch WorkaroundSplinter Review
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.
> ###!!! 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.
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.
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.
Neeti, do you want to add a unit test for this using your test infrastructure?
Flags: in-testsuite?
Attached patch FixSplinter Review
Assignee: nobody → bzbarsky
Attachment #376452 - Flags: superreview?(dbaron)
Attachment #376452 - Flags: review?(dbaron)
Why isn't the interrupt that interrupted within the reflow of the kids sufficient for marking things dirty?
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.
Attachment #376452 - Flags: superreview?(dbaron)
Attachment #376452 - Flags: superreview+
Attachment #376452 - Flags: review?(dbaron)
Attachment #376452 - Flags: review+
Comment on attachment 376452 [details] [diff] [review]

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

Pushed with what I hope are improved comments.
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 499138
This landed before the 1.9.2 branchpoint.
Keywords: fixed1.9.2
Target Milestone: --- → mozilla1.9.2a1
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)
You need to log in before you can comment on or make changes to this bug.