Closed
Bug 475548
Opened 15 years ago
Closed 15 years ago
Apache directory index page flashes black on refresh
Categories
(Core :: Layout, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: jmjjeffery, Assigned: zwol)
References
()
Details
(Keywords: fixed1.9.1, regression)
Attachments
(1 file, 3 obsolete files)
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
Comment 1•15 years ago
|
||
Changesets in regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=44fd2796cee1&tochange=0dec4e077be2 Almost certainly bug 474201.
Assignee | ||
Comment 2•15 years ago
|
||
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).
Reporter | ||
Comment 3•15 years ago
|
||
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
Comment 4•15 years ago
|
||
(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.
Comment 5•15 years ago
|
||
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
Assignee | ||
Comment 6•15 years ago
|
||
(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.
Assignee | ||
Comment 7•15 years ago
|
||
(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.
Assignee | ||
Comment 8•15 years ago
|
||
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.
Assignee | ||
Comment 10•15 years ago
|
||
(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)
Assignee | ||
Updated•15 years ago
|
Attachment #359144 -
Attachment is obsolete: true
Comment 11•15 years ago
|
||
(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.
Comment 12•15 years ago
|
||
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?
Assignee | ||
Comment 13•15 years ago
|
||
(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.
Assignee | ||
Comment 15•15 years ago
|
||
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+
Comment 17•15 years ago
|
||
(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.
OOps ... Zack, can you post an updated patch for landing?
Keywords: checkin-needed
Whiteboard: [needs landing]
Blocks: 474886
Assignee | ||
Comment 19•15 years ago
|
||
Here it is. Sorry for the delay.
Attachment #359436 -
Attachment is obsolete: true
Attachment #359650 -
Flags: superreview+
Attachment #359650 -
Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/02dd72d71d83
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Comment 21•15 years ago
|
||
(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
Comment 22•15 years ago
|
||
(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.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/95813ca07013
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Depends on: 486473
You need to log in
before you can comment on or make changes to this bug.
Description
•