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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

P1
normal
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: wgianopoulos, Assigned: bzbarsky)

Tracking

({regression})

Trunk
mozilla1.9.2a1
x86
Windows XP
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
in-testsuite ?

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Aha!  I tend to use the expanded view.  I'll try this with the list view.
Flags: blocking1.9.2+
Priority: -- → P1
(Reporter)

Comment 2

10 years ago
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.
(Reporter)

Comment 3

10 years ago
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.
(Reporter)

Comment 4

10 years ago
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
(Reporter)

Comment 5

10 years ago
I neglected to mention in comment #4 that this was with a Linux debug build.
(Reporter)

Comment 6

10 years ago
Created attachment 376025 [details] [diff] [review]
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.
(Assignee)

Comment 7

10 years ago
> ###!!! 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.
(Assignee)

Comment 8

10 years ago
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.
(Assignee)

Comment 9

10 years ago
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.
(Assignee)

Comment 10

10 years ago
Neeti, do you want to add a unit test for this using your test infrastructure?
Flags: in-testsuite?
(Assignee)

Comment 11

10 years ago
Created attachment 376452 [details] [diff] [review]
Fix
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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?
(Assignee)

Comment 13

10 years ago
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]
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
(Assignee)

Comment 15

10 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/42a3db8c8837 with what I hope are improved comments.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Depends on: 499138
This landed before the 1.9.2 branchpoint.
Keywords: fixed1.9.2
(Assignee)

Updated

9 years ago
Target Milestone: --- → mozilla1.9.2a1
status1.9.2: --- → beta1-fixed
Keywords: fixed1.9.2
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.