1083072 - Logo image shows up randomly after refreshing the webpage

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Jean-Marc]

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).

Comment 1

[Loic]

I'm able able to reproduce it with FF33 and FF35. I think it's an issue about cache too (maybe cache v2).

Comment 2

[Jean-Marc]

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

Comment 3

[Loic]

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.

Comment 4

[Jean-Marc]

Sorry :-)
It's ok, I've put the older code back again -- the logo shows up randomly on FF32, etc.

Comment 5

[Loic]

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.

Comment 6

[Jean-Marc]

Do you consider this bug as "confirmed" yet ? Can I put the fixed-code back again ?

Thanks in advance,
Best regards

Comment 7

[Loic]

Yes, I confirm it. It would be nice to have the testcase "working" until a Moz dev check the issue. I'll CC someone.

Comment 8

[Jean-Marc]

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

Comment 9

[John Schoenick [:johns]]

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?

Comment 10

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 11

[Jean-Marc]

Thank you guys, somehow I woke up this morning feeling useful :)

Comment 12

[Loic]

[Tracking Requested - why for this release]: recent regression since FF32, but it could be maybe fixed in FF34 or later.

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

(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+?

Comment 14

[Loic]

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.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Given that this has already been fixed on the site, I'm not going to track. I have marked 33-36 as affected.

Comment 16

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 17

[Jean-Marc]

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.

Comment 18

[Jean-Marc]

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.

Comment 19

[Alice0775 White]

Attachment: 20150319085844.zip

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

Comment 20

[Alice0775 White]

[Tracking Requested - why for this release]: 
This is regressed since Firefox32
It should be fixed on next ESR38.

Comment 21

[Seth Fowler [:seth] [:s2h]]

We actually tracked this down recently. Bug 1141376 and bug 1019840 together should fix this.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 38 and 39.

Comment 23

[Seth Fowler [:seth] [:s2h]]

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.

Comment 24

[Seth Fowler [:seth] [:s2h]]

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.

Comment 25

[Seth Fowler [:seth] [:s2h]]

We really need verification of the fix here. Can anyone who could reproduce this problem previously confirm that it's fixed?

Comment 26

[Jean-Marc]

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

Comment 27

[Loic]

(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.

Comment 28

[Loic]

(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.

Comment 29

[Seth Fowler [:seth] [:s2h]]

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.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

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!

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Seth, can we uplift that to 38? Thanks

Comment 32

[Seth Fowler [:seth] [:s2h]]

(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.