Closed Bug 1083072 Opened 10 years ago Closed 10 years ago

Logo image shows up randomly after refreshing the webpage

Categories

(Core :: Graphics: ImageLib, defect)

32 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox33 --- affected
firefox34 --- affected
firefox35 --- affected
firefox36 --- affected
firefox37 --- affected
firefox38 + fixed
firefox39 --- verified
firefox-esr31 --- unaffected

People

(Reporter: jmdelprato, Assigned: seth)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.13 Safari/537.36 Steps to reproduce: I'm currently working on a WordPress website (WP 3.9.2) and I'm loading my main logo with a simple <img /> tag as you can see on this page : http://showbuzz.org/tell-me-more/?page_id=84 On the Firefox 32 branch, this image shows up randomly on each refresh. I don't encounter this issue on Firefox 31 and below. As soon as I open up the inspector, the image appears immediately which makes it difficult to debug. Actual results: - The image is randomly loaded, the page actually seems to stop rendering before it loads the image. Expected results: - The logo should appear on every refresh (as it does on Firefox 31 and below or other browsers).
I'm able able to reproduce it with FF33 and FF35. I think it's an issue about cache too (maybe cache v2).
Component: Untriaged → Networking: Cache
Product: Firefox → Core
Summary: With Firefox 32.0, my logo image shows up randomly. Some refreshes fully load it, some don't (it seems like a cache issue) → Logo image shows up randomly after refreshing the webpage
Just a quick comment : Since the issue only shows up with the main logo, I looked at its particularities --- it's a WordPress install, and the logo isn't hardcoded : it is loaded with a PHP call. <div class="site-logo"> <a href="http://www.showbuzz.org/tell-me-more/index.php?accueil=1" title="<?php echo esc_attr( get_bloginfo( 'name', 'display' ) ); ?>" rel="home"> <img data-altlogo="<?php echo $alternate_logo; ?>" src="<?php echo $sitelogo; ?>" alt="<?php echo esc_attr( get_bloginfo( 'name', 'display' ) ); ?>"> </a> </div> As you can see, there's a "data-altlogo" attribute before the actual "src" within the <img> tag. For the sake of simplicity, I've hardcoded the src attribute and deleted the "data-altlogo" attribute. Upon dozens of refreshes, I haven't yet been able to reproduce the issue yet on FF32, FF33 and FF34 :-) Can you confirm that you cannot reproduce the bug anymore ? Would you like me to put the older code live again in order to debug it ? Best regards
Argh, if you fixed on your side, we can't know if it's a bug in Firefox. :D My guess is it's a bug, I tried with previous versions and the logo was loaded at each refresh. Could you cancel your change if possible? I'll like to run mozregression.
Flags: needinfo?(jmdelprato)
Sorry :-) It's ok, I've put the older code back again -- the logo shows up randomly on FF32, etc.
Flags: needinfo?(jmdelprato)
Regression range: good=2014-05-31 bad=2014-06-01 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=323156681cef&tochange=d6fccb7c56a8 My guess is it's a side-effect of bug 870021.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Do you consider this bug as "confirmed" yet ? Can I put the fixed-code back again ? Thanks in advance, Best regards
Yes, I confirm it. It would be nice to have the testcase "working" until a Moz dev check the issue. I'll CC someone.
Flags: needinfo?(jschoenick)
Hi again ! The website is intented to go to production tomorrow so I might reactivate the correct code on 11am. If you want to test that bug again, I could duplicate its current state and give you another URL. Don't hesitate ! Best regards
Bug 870021 regression from: https://hg.mozilla.org/mozilla-central/rev/4a326b265f41 What's happening is: - nsImageFrame is created, calls OnStartContainer(). At this point the image has imgIRequest::STATUS_SIZE_AVAILABLE, so we set our intrinsic size - we hit ComputeSize, and ask the imageLoader for the natural size of the image, except the request suddenly does NOT have size available and is 0x0, so we get 0x0. - we get a Notify(SIZE_AVAILABLE) and call OnStartContainer again, but since the intrinsic sizes didn't change between init and now, we don't invalidate the frame, so we never update the new natural size in ComputeSize The patch above was assuming image natural size would only change when intrinsic size did, but we seem to go through | size available -> not -> available again | states with the same request (some cache black magic at the network layer?), which means its not safe to rely on size being available in ComputeSize. @seth - do you know why this might be happening, and if it suggests there's a deeper bug? Does it sound reasonable to just cache natural size in nsImageFrame::UpdateIntrinsicSize?
Blocks: srcset
Component: Networking: Cache → ImageLib
Flags: needinfo?(jschoenick) → needinfo?(seth)
(In reply to John Schoenick [:johns] from comment #9) > @seth - do you know why this might be happening, and if it suggests there's > a deeper bug? Does it sound reasonable to just cache natural size in > nsImageFrame::UpdateIntrinsicSize? It sounds like there's a deeper bug somewhere, absolutely. We should try to fix that rather than working around it. Once we have a size available, it should stay available and it shouldn't change, except in the case of multipart/x-mixed-replace images. (And it doesn't seem like that's what we're dealing with here.) Without investigating more deeply, I'm not sure precisely where things are going wrong. It'd be useful to verify that the current request on nsImageLoadingContent never gets changed and that imgRequestProxy::ChangeOwner never gets called on the imgRequestProxy associated with the request (in other words, on the imgIRequest object). If those things are true, I'd want to understand why we fail to get the natural size - presumably RasterImage::GetWidth is failing, but I don't understand how it could possibly ever recover in that case, since it only fails if RasterImage::mError is true, and once we set it to true we never unset it.
Flags: needinfo?(seth)
Thank you guys, somehow I woke up this morning feeling useful :)
[Tracking Requested - why for this release]: recent regression since FF32, but it could be maybe fixed in FF34 or later.
(In reply to Loic from comment #12) > [Tracking Requested - why for this release]: recent regression since FF32, > but it could be maybe fixed in FF34 or later. Before tracking, can you confirm that this issue impacts 34+?
Yes, in general, I don't report a regression already fixed in advanced versions. ;) As the reporter said, the website will go in production and he has probably fixed the issue with the workaround in comment #2, but the underlying issue is still present.
Given that this has already been fixed on the site, I'm not going to track. I have marked 33-36 as affected.
(In reply to Jean-Marc from comment #11) > Thank you guys, somehow I woke up this morning feeling useful :) Thanks for reporting this! You had offered to duplicate the version of the site that demonstrated the bug so that it would remain accessible; could you please do that if you haven't already? This is definitely something we want to fix, but I'm guessing it may take some time to debug.
Hi, thank you for your kind comment, I have indeed updated the code in order to fix the issue (cf. comment #2). I will duplicate the site on another server (with the original, error-prone code) and I will publish a test-URL by tomorrow.
Flags: needinfo?(jschoenick)
Just a quick note : The site went finally on production yesterday, on another web server. I've put the original code back on the development server, accessible through the URL I've submitted in the first post ... and it will stay there as long as you need it ! With FF 32 and now FF 33.0.2, the URL http://showbuzz.org/tell-me-more/?page_id=84 randomly shows the logo on the upper left corner of the page.
Attached file 20150319085844.zip (deleted) —
Steps to reproduce: 1. Open attached index.html 2. Reload(F5), repeat step2 if necessary Actual results: Image disappears Expected results: Image should not disappear. Page layout should be consistent after reload Regression window https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8164fe57ac92&tochange=9ae055051998 Suspect: Bug 870021
[Tracking Requested - why for this release]: This is regressed since Firefox32 It should be fixed on next ESR38.
We actually tracked this down recently. Bug 1141376 and bug 1019840 together should fix this.
Depends on: 1141376, 1019840
Flags: needinfo?(john)
OK, bug 1141376 and bug 1019840 both landed this morning. (And will be in the next Nightly.) So we just need to verify the fix and then uplift.
Assignee: nobody → seth
Jean-Marc, I'm sorry it took so long to get this fixed. This was a tricky one. The fix should be in the Nightly that comes out tonight. Could you verify that it's fixed now? I can't reproduce the issue here.
Flags: needinfo?(jmdelprato)
We really need verification of the fix here. Can anyone who could reproduce this problem previously confirm that it's fixed?
Keywords: verifyme
Sorry for my late answer and many thanks for taking the time to address this issue...! Under FF 36.0.4, I'm not able to reproduce the problem any more upon many refreshes (that was actually the problem -- it showed up randomly). Would you like me to test it on other versions as well ? Thanks
Flags: needinfo?(jmdelprato)
(In reply to Jean-Marc from comment #26) > Sorry for my late answer and many thanks for taking the time to address this > issue...! > Under FF 36.0.4, I'm not able to reproduce the problem any more upon many > refreshes (that was actually the problem -- it showed up randomly). Would > you like me to test it on other versions as well ? > Thanks The original issue is only fixed in FF39 (Nightly) at the moment, but your testcase has been fixed by... yourself according to your comment #8. :) You should test the old version of your website, before he went to production, with FF39 if you really want to confirm it's fixed or not.
(In reply to Seth Fowler [:seth] from comment #25) > We really need verification of the fix here. Can anyone who could reproduce > this problem previously confirm that it's fixed? Working range according to the testcase posted by Alice: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cf1060d8ce9f&tochange=4d2d97b3ba34 Your 2 bugs are present, so it's fixed for me.
Excellent! Thanks for the verification, Loic. Jean-Marc, if you get a chance to test against the old version of that website, more verification is always better. I know that may be tricky, though. I'm going to go ahead and mark this as resolved.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Keywords: verifyme
Looks like patches on bug 1141376 and bug 1019840 should land soon on aurora. Since this is resolved for 39 I'll untrack it for 39 and leave it tracked for 38. Nice to see this tricky issue resolved!
Seth, can we uplift that to 38? Thanks
Flags: needinfo?(seth)
(In reply to Sylvestre Ledru [:sylvestre] from comment #31) > Seth, can we uplift that to 38? Thanks It's already in 38. There's no patch in this bug; the uplift happened in bug 1141376 and bug 1019840.
Flags: needinfo?(seth)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: