269402 - [FIXr]After logging in to Fidelity, page is rendered but not visible

Status: VERIFIED FIXED

(Core :: Web Painting, defect, P1)

Description

[Curtis Williams]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041111
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041111

This is a change in behaviour in builds for about the last two weeks.  At the
moment, I am not sure which of the nightly builds this stopped working, but I
will investigate and update this as soon as I can figure out which was the last
good build.

After going through a successful login to http://www.401k.com, Fidelity's web
site, a summary page should be displayed for my account.  What I am presented
with is a page that is not visible, but all of the elements seem to be there. 
For instance, if I move the cursor around the page, it will turn into the finger
over top of the places where links and buttons should be, but the page is
entirely invisible.  Because this page is rendered in IE, I know where the
logout button is, and can click on it and successfully logout.  So the elements
are there and functioning, just not visible.

I will be attaching a saved View | Page Source of the page in question.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Curtis Williams]

Attachment: View Page Source

Comment 2

[Curtis Williams]

Looks like the last good trunk build is 2004102906.  I am also attaching a
screen shot of what the page should look like when properly rendered.

Comment 3

[Curtis Williams]

Attachment: properly rendered screenshot

Comment 4

[Boris Zbarsky [:bzbarsky]]

Looks like fallout from bug 244290...  I see the following in a debug build on
the "view page source" attachment (which doesn't paint the error pages like it
should):

###!!! ASSERTION: Invalid batch count!: 'mUpdateBatchCnt >= 0', file
/home/bzbarsky/mozilla/xlib/mozilla/view/src/nsViewManager.cpp, line 3335
###!!! ASSERTION: Cannot process pending updates with refresh disabled:
'mRefreshEnabled', file
/home/bzbarsky/mozilla/xlib/mozilla/view/src/nsViewManager.cpp, line 1607

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ok, I can fix the issue with the "View Page Source" attachment (which may not be
the same as the original bug, but I suspect it is...) by not setting
mRootViewManager to |this| when mRootView is set to null by the destructor of
said view.

This does look like it causes us to possibly have a dangling mRootViewManager
pointer in the child viewmanager.  On the other hand, it looks like there are
cases when our root view is killed off inside of a view update batch.  In
particular, ProcessReflowCommands() can call UnsuppressAndInvalidate(), which
can call Show() on a document viewer, which kills off the old document viewer
(along with the frame tree, the old root view, the presshell that was processing
reflow commands, etc; see comments in UnsuppressAndInvalidate() after the call
to Show()).  If this is all happening in a subframe, we won't enable painting
for the overall viewmanager tree...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Actually, more like this

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 166302 [details] [diff] [review]
Actually, more like this

It's a little messy, but as long as people are messing with our root view.... 
This holds a ref to mViewManager if it's not |this| and makes sure to never
unset mViewManager.

I'm tempted to release mRootViewManager in SetRootView() if it's not null and
not |this| and aView is non-null.  Or at least to assert that we don't have an
mRootViewManager there...

Comment 9

[Boris Zbarsky [:bzbarsky]]

I think it's worth taking this for 1.8a5...

Robert, thoughts on the NS_RELEASE/assert part of comment 8?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 166302 [details] [diff] [review]
Actually, more like this

This fixes a bunch of painting regressions on pages with frames...  I don't
think we want to ship 1.8a5 with those.

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 166302 [details] [diff] [review]
Actually, more like this

a=asa for 1.8a5 checkin.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 13

[Curtis Williams]

I had been out on vacation, so I hadn't had a chance to check any recent builds.

Just wanted to add a short note to confirm that this problem is resolved with
last night's build.

Thanks!

Comment 14

[Boris Zbarsky [:bzbarsky]]

Curtis, thanks for confirming that that patch fixed the issue!