Closed Bug 552538 Opened 15 years ago Closed 13 years ago

CSS sprites loaded via redirects do not work correctly; on first rollover image, flashes to background

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME
Tracking Status
blocking2.0 --- -
status1.9.2 --- ?

People

(Reporter: webmaster, Assigned: dbaron)

References

(Depends on 1 open bug, )

Details

(Keywords: regression, testcase, Whiteboard: [should be fixed by bug 552605 (when it is fixed)][should work around for 2.0])

Attachments

(2 files)

User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6 Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6 When you rollover an image, the site background "flashes" before the rollover image appears. This issue is not present in Firefox 3.5.8. Please note that CSS sprites are being used and that the rollover image is the same as the "original" state image. Reproducible: Always Steps to Reproduce: 1. Go to http://www.1031pictures.com/index2.php 2. Mouse over one of the images. You will see that the black background flashes in between. Actual Results: When you rollover an image, the site background "flashes" before the rollover image appears. Please note that CSS sprites are being used and that the rollover image is the same as the "original" state image. Expected Results: The black background should not appear in between. This issue is not present in Firefox 3.5.8.
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a3pre) Gecko/20100315 Minefield/3.7a3pre Confirmed on Windows Vista.
Component: General → Style System (CSS)
OS: Mac OS X → All
Product: Firefox → Core
QA Contact: general → style-system
Hardware: x86 → All
Version: unspecified → Trunk
I don't see it in Linux or Windows 7. Is it a really fast flicker that I need a slow computer to see it with? Does it only occur while the page is loading or always? I tried mousing over everything on that page during and after page load and never saw a flicker. I tested with these Linux builds and a similar set in Windows. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a4pre) Gecko/20100324 Minefield/3.7a4pre Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2
The issue was "fixed" on the original site by making a simple change to the CSS. The original CSS code worked fine in previous versions of Firefox. Original CSS: http://img.skitch.com/20100401-fskkes7ih39gis1bynu7yx6bcd.jpg New CSS that does not have this issue: http://img.skitch.com/20100401-nujcwuhhg7epkjar5spytyg2r1.jpg I have uploaded the original page and css here in case further troubleshooting needs to be done as other designers/Firefox users may be experiencing the same issue. http://www.1031pictures.com/firefoxtest/index2.php
Thanks for the page to showcase the issue. I can reproduce with it now. Regression range: works: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090219 Minefield/3.2a1pre http://hg.mozilla.org/mozilla-central/rev/d9d4bf676f65 broken: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090220 Minefield/3.2a1pre http://hg.mozilla.org/mozilla-central/rev/35d78a340566 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d9d4bf676f65&tochange=35d78a340566
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file testcase using proxy (deleted) —
Saving locally didn't really work so I used a proxy. Reproduces every time for me. The html (or php) page files is in the "log" directory. STR (I used linux, so adjust accordingly): 1. Extract testcase somewhere 2. Create new firefox profile 3. Open firefox, go to http://www.1031pictures.com/firefoxtest/index2.php 4. "cd" into "http-recording-proxy" directory in terminal 5. Run proxy with "./TinyHTTPProxy.py --replay" (note HTTP proxy and port) 6. In firefox, Edit->Preferences->Advanced->Network->Connection->Settings (probably Tools->Options->... in windows). 7. Click "Manual Proxy Configuration" 8. Set "HTTP Proxy:" to "0.0.0.0" "Port:" to "8000" (unless step 5 gave you something different) "Check" "Use this proxy server for all protocols" 9. Click "OK" and "Close" to exit options menus. 10.Reload (override cache) with CTRL-F5 or CTRL-SHIFT-R
Keywords: testcase-wanted
Keywords: testcase
This bug still appears in recent nightlies. The link given by the reporter to troubleshoot is down but the testcase still works. It would be good to remove the proxy dependency but I don't know how to do that. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a5pre) Gecko/20100603 Minefield/3.7a5pre
Hmm. This looks like possible fallout from the imageloader changes in that regression range....
Blocks: 322475
I was about to say the same thing. Could also be related to bug 321361 (which touched image-caching code), but bug 322475 seems more likely to be the cause.
OK, I can reproduce with the attached replay testcase; the key seems to be the redirect, which causes image cache misses due to bug 552605. So when we go into :hover we download the image again.
Depends on: 552605
Attached file Testcase showing the problem (deleted) —
This just uses a script that sleeps before replying as the redirect target.
OK, using both that testcase and the proxy testcase, local builds show the problem as first appearing with http://hg.mozilla.org/mozilla-central/rev/8bf2088734ac
And in particular, since the background URI doesn't change we used to bail out early from nsImageLoader::Load (called from PaintBackgroundWithSC) in the old code and do nothing (so hold on to the old imgIRequest, which was already loaded). But in the new code, we no longer do that check in nsImageLoader::Load, and it wouldn't help anyway since we create a new nsImageLoader on every paint in any case. So we use the new nsImageLoader, which points to the _new_ imgIRequest, which is not done loading yet, because it missed the image cache due to bug 552605.
blocking2.0: --- → ?
status1.9.2: --- → ?
David, see comment 12.
Assignee: nobody → dbaron
Summary: CSS sprites do not work correctly; on first rollover image, flashes to background → CSS sprites loaded via redirects do not work correctly; on first rollover image, flashes to background
Whiteboard: [should be fixed by bug 552605 (when it is fixed)]
Depends on: 606311
Whiteboard: [should be fixed by bug 552605 (when it is fixed)] → [should be fixed by bug 552605 (when it is fixed)][should work around for 2.0]
I've been poking at fixing this, but I think decode-on-draw has made it a bit harder to fix. In particular, I can make it so we don't recreate a new image loader (which is slightly different from what happened before, but should be equivalent). But when I do that, we end up with ImageRenderer::PrepareImage returning false because mImage->IsComplete() is false... which just results in the patch making things worse since we don't get any followup notification (I suspect because it means it means the frame's background image loaders's imgRequestProxy is now cloned from a different imgRequest than the imgRequestProxy in the style data, although I haven't confirmed this).
Some email between me and bz (all times EST), attached with permission: On 12/30/10 8:01 AM, L. David Baron wrote: > I'd be interested if you have any thoughts on > https://bugzilla.mozilla.org/show_bug.cgi?id=552538#c14 On Thursday 2010-12-30 16:05 -0800, Boris Zbarsky wrote: > Erm. Why would we be cloned off a different imgRequest? > > Past that, I have no idea what decode-on-draw is doing here. :( On 12/30/10 6:29 PM, L. David Baron wrote: > Er, I guess you're saying the image cache should have taken care of > that, and there should only ever be one imgRequest for a given URL? > (despite the image cache's brokenness in this case) > > I guess I should debug this in more detail with the patch applied. On Thursday 2010-12-30 19:08 -0800, Boris Zbarsky wrote: > Oh, I see what you mean. No, if we call loadImage again we'll get a > different imgRequest. But I thought we just cloned from the style > data's imgRequestProxy instead of making a separate loadImage call. > Do we not? On 12/30/10 7:17 PM, L. David Baron wrote: > But there's two different style rules, since we have, basically: > p { background: url(foo) 0px 0px } > p:hover { background: url(foo) 0px 100px } > so we have two different nsCSSValues. > > This means the patch to preserve the imgRequestProxy when the style > changes but the background-image URL is the same means we end up > still using the imgRequestProxy cloned off the style data derived > from the p rule's nsCSSValue even when the current style data have > an imgRequestProxy from the p:hover rule's nsCSSValue. On Thursday 2010-12-30 19:34 -0800, Boris Zbarsky wrote: > On 12/30/10 7:17 PM, L. David Baron wrote: > >But there's two different style rules, since we have, basically: > > p { background: url(foo) 0px 0px } > > p:hover { background: url(foo) 0px 100px } > >so we have two different nsCSSValues. > > Ah, ok. Then yes, img cache brokenness means there are two > different imgRequests here. > > >This means the patch to preserve the imgRequestProxy when the style > >changes but the background-image URL is the same means we end up > >still using the imgRequestProxy cloned off the style data derived > >from the p rule's nsCSSValue even when the current style data have > >an imgRequestProxy from the p:hover rule's nsCSSValue. > > Yes, agreed. What causes the problem at that point? On 12/30/10 7:57 PM, L. David Baron wrote: > ImageRenderer::PrepareImage, using the image from the style data > (i.e., from p:hover) calls nsStyleImage::IsComplete, which calls > imgRequestProxy::GetImageStatus, which I believe returns a status > indicating the imgRequest hasn't loaded yet, causing IsComplete to > return false (I confirmed that it returns false). At that point > PrepareImage calls RequestDecode and then returns false, and we > don't draw anything. > > But the notifications from this load don't actually go to the frame, > since the frame is still set up to receive notifications from the > image cloned from the p rule, since my patch to work around the > caching bug explicitly didn't re-clone since the URL was the same. On Thursday 2010-12-30 20:00 -0800, Boris Zbarsky wrote: > Oh, I see. Ugh. So we could re-clone if the old image is not > complete, I suppose.... Or we could get imagelib to fix their bug. > :( On 12/30/10 8:24 PM, L. David Baron wrote: > Alternatively, we could just ship the bug in a second release (it's > already in 3.6). On Thursday 2010-12-30 20:30 -0800, Boris Zbarsky wrote: > Imho, we should do just that, esp. if we plan to actually ship again > 3 months later. The amount of effort it would take to work around > the imagelib brokenness on the layout side is huge. In summary, we left this as a blocker (after minusing its dependency) because we thought working around that dependency would be easy. However, decode-on-draw has (as far as we can tell) made it much harder to work around that dependency, so I tend to think this should no longer block (or, if it should, we should make bug 552605 block and fix it that way). We did ship this in 3.6, and we can still try to fix it (bug 552605) for an upcoming release in 2011. So changing blocking2.0:final+ back to ? for re-evaluation.
blocking2.0: final+ → ?
And, for the record, my current attempt at a workaround is: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/bcbfae0ce6c9/reuse-image-loader I probably want to land the comment change in nsPresContext::SetupBorderImageLoaders and perhaps change the code in it as well. (Right now it sometimes creates a loader for a null image.)
This is fixed. I tested with both Firefox 10 and latest trunk. Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120309 Firefox/13.0a1 Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Indeed! Likely implicitly fixed when I fixed bug 552605.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: