Closed Bug 475548 Opened 15 years ago Closed 15 years ago

Apache directory index page flashes black on refresh

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: jmjjeffery, Assigned: zwol)

References

(
URL
)

Details

(Keywords: fixed1.9.1, regression)

Attachments

(1 file, 3 obsolete files)

rev 3: NS_ASSERTION instead of NS_ABORT_IF_FALSE
2.81 KB, patch
zwol
: review+
zwol
: superreview+
Details | Diff | Splinter Review 
On a ftp page with a long list such as:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/
when you go to the bottom of the page and refresh the page the screen flashes black during the refresh.

Strangely though, if you only go about 1/2 way down page and refresh it does not flash the black screen during refresh. 

I've traced back a regression range:

0121 h1701 OK
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090121 Minefield/3.2a1pre Firefox/3.0.4 ID:20090121153254
changeset: http://hg.mozilla.org/mozilla-central/rev/44fd2796cee1

0122 h0325 Bad
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090122 Minefield/3.2a1pre Firefox/3.0.4 ID:20090122015406
changeset: http://hg.mozilla.org/mozilla-central/rev/0dec4e077be2
Blocks: 474201
Not reproducible on Linux, but ... did you mean to use an http URL?  There's a big difference between the bug being visible on http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ (an issue with refreshing very long pages in general) and being visible on ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ (perhaps just an issue with the synthetic DOM generated for ftp: URLs).
Yes, the http is one I use to check for new nightly's, or older builds for testing.
Summary: ftp page flashes black on refresh → Apache directory index page flashes black on refresh
(In reply to comment #2)
> Not reproducible on Linux,

I can reproduce it 100% on Linux with

> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/

but not with

> ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090127
Firefox/3.2a1pre

Built from http://hg.mozilla.org/mozilla-central/rev/13e95c7ff78b

on Ubuntu 8.10, though the issue is IMVHO rather minor in comparison to the bug
that has regressed this.
I too can reproduce this on Linux, as described in comment 0, at http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ using today's nightly. 
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090127 Minefield/3.2a1pre
Built from http://hg.mozilla.org/mozilla-central/rev/4a34b7ff26e2

The only slight difference is that I get a flash of weird graphical corruption, rather than a flash of blackness.

Platform --> All
OS: Windows Vista → All
(In reply to comment #5)
> I too can reproduce this on Linux, as described in comment 0, at
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ using today's nightly. 

Ok, I guess I wasn't trying hard enough.  Will experiment further.  (This is actually good -- it is enormously easier for me to fix bugs that manifest on Linux.)

> The only slight difference is that I get a flash of weird graphical corruption,
> rather than a flash of blackness.

That means the flash is a time period where we haven't painted (that area of) the window at all.  Linux gives you garbage when that happens, Windows paints it black.
(thinking out loud) There's nothing special formatting-wise about an Apache index page.  This ought to be reproducible on any very long page with no background specified for the <html> or <body> elements.  And I bet I even know what's wrong: the fix for bug 474201 was correct *except* that we can't be setting the view manager's default background color to something transparent.  Or possibly it needs to get made opaque when the docshell copies it on page load -- I recall there being some reason why it *had* to be transparent.  Oh right, bug 469170.  Gah, bug 469170.
I can now reproduce the bug, if I use an enormous page (like a tinderbox full build log), I continuously resize the window while the page is reloading, and I get lucky.

The flash of garbage is simultaneous with this dumped to stderr:

WARNING: nsViewManager: Asked to paint a default background, but no default background color is set!: file /home/zack/src/mozilla/moz-central/view/src/nsViewManager.cpp, line 553

Line 553 is nsViewManager::DefaultRefresh(); I'm taking that as evidence for the hypothesis in comment 7.  And indeed the attached patch (which is a horrible kludge) appears to suppress the problem.  It would be great if people who see the bug more easily than I do could test it.

This is not a fix, for two reasons.  First, it's just blindly substituting white if the old color was even a little transparent.  We ought to use NS_ComposeColors with the pres context default (guaranteed opaque) underneath, but if I do that, libdocshell.so fails to link.

Second, there are other conditions where we might reach DefaultRefresh() with the view manager's background transparent.  I'd like to force the view manager background to opaque unconditionally, but I fear that will regress bug 469170.  Have I mentioned that I didn't think bug 469170 was legitimate?
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Maybe in DefaultRefresh we should just composite the given default background color onto white.
Attached patch working patch as suggested (obsolete) — — Splinter Review 
(In reply to comment #9)
> Maybe in DefaultRefresh we should just composite the given default background
> color onto white.

That seems to work well -- no garbage on screen (with a temporary printf to ensure that DefaultRefresh is called) and doesn't regress bug 469170.  It might be better if we could use the prescontext default instead of a fixed white, but I don't think we have access to a prescontext from DefaultRefresh.  I thought about forcing all setters of view manager default background to pass an opaque color but that winds up being messier.

I have no idea how to write an automated test for this.
Attachment #359310 - Flags: review?(roc)
Attachment #359144 - Attachment is obsolete: true
(In reply to comment #10)
> Created an attachment (id=359310) [details]
> working patch as suggested

This patch fixes the problem as described if your browser default background color is set to white.  However if your browser default background color is set to black, then it makes the page flash white on reload.

This a really needs to be using the configured browser default background color from the browser.display.background_color preference, rather than always white.
The only pages I have seen that show this flashing, all have no background color specified so end up using the browser configured default background color.  Is that the only case where this issue occurs?
(In reply to comment #11)
> 
> This patch fixes the problem as described if your browser default background
> color is set to white.  However if your browser default background color is set
> to black, then it makes the page flash white on reload.

I was afraid of that.  The problem is that the user-configured default background is stored in the nsPresContext object, and I don't think we have access to one of those when nsViewManager::DefaultRefresh is called.  Unless someone knows better than me, that means it's going to have to be on the callers of nsViewManager::SetDefaultBackground to composite in the nsPresContext default as necessary -- and it gets worse, because one of those places is nsDocShell, which cannot call NS_ComposeColors!  (But we might be okay there because it's copying data from one view manager to another, so it can maybe rely on the *other* callers to do the right thing.)

(In reply to comment #12)
> The only pages I have seen that show this flashing, all have no background
> color specified so end up using the browser configured default background
> color.  Is that the only case where this issue occurs?

Essentially, yes.  If the page style specifies an opaque background color, that will get used by DefaultRefresh instead of the browser default.
As discussed on IRC, we should just get the prescontext's default color via a method added to nsIViewObserver.
Both of the places that set the view manager's default background to a new value have access to the prescontext, so it seemed better to me to put the load on them; that way we don't have to change any interfaces  This patch does that, and also adds some defensive coding to DefaultRefresh in case it ever gets called wrong (e.g. between when the view manager is created and the document viewer sets its default background).
Attachment #359310 - Attachment is obsolete: true
Attachment #359436 - Flags: review?(roc)
Attachment #359310 - Flags: review?(roc)
Comment on attachment 359436 [details] [diff] [review]
rev 2: ensure view manager default background is opaque

Use NS_ASSERTION here instead of NS_ABORT_IF_FALSE. The world isn't going to end if a transparent background color is used; forcing the world to end will make debugging much less pleasant.
Attachment #359436 - Flags: superreview+
Attachment #359436 - Flags: review?(roc)
Attachment #359436 - Flags: review+
(In reply to comment #15)
> Created an attachment (id=359436) [details]
> rev 2: ensure view manager default background is opaque

This seems to work regardless of what I set for default background color.
Flags: blocking1.9.1+
Keywords: checkin-needed
Priority: -- → P2
Whiteboard: [needs landing]
OOps ... Zack, can you post an updated patch for landing?
Keywords: checkin-needed
Whiteboard: [needs landing]
Here it is.  Sorry for the delay.
Attachment #359436 - Attachment is obsolete: true
Attachment #359650 - Flags: superreview+
Attachment #359650 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/02dd72d71d83
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
(In reply to comment #20)
> http://hg.mozilla.org/mozilla-central/rev/02dd72d71d83

Does not apply to 1.9.1:
{
patching file layout/base/nsCSSRendering.cpp
Hunk #1 FAILED at 1172
1 out of 1 hunks FAILED
}
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.2a1
(In reply to comment #21)
> Does not apply to 1.9.1:

That's because it modifies code added in bug 473398 (the bug that caused this regression).  After applying that bug's patch to 1.9.1, this bug's patch applies fine.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: