764337 - Firefox 14+ requests images inserted by JS multiple times if not cached

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[strayer]

Attachment: Fiddler2 Screenshot showing multiple requests

I visited http://www.crazy-factory.com/product_listing_ng.php?cat_id=316 and noticed that the product images load very slow.

After investigating with Firebug and Fiddler2 I noticed that the product images inserted by JavaScript are requested multiple times, blocking each other. As soon as they are locally cached the problem doesn't occur anymore.

This only happens in Firefox 14 and above, Firefox 13 shows no problems. This was tested in a clean Windows XP VM with clean profiles. Versions:
Firefox Stable 13.0 (working as expected)
Firefox Beta 14.0
Aurora 15.0a2 (2012-06-12)
It does not happen in IE9 and Chrome 21.

Sadly I was not able to create a reduced test case - all reduced test cases that I ran locally didn't have that problem and requested the images just fine from the original server.

If needed, all minified JavaScript files are also available uncompressed on the server, just remove ".min" from the URL.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Steve, would you be willing to find the day that this broke using http://harthur.github.com/mozregression/ ?

I can totally understand if you don't want to commit to that, by the way.  Just let me know.

Comment 2

[strayer]

I was bored, so I fiddled with the mozregression tool.

It looks like this broke in the nightlies between 2012-03-23 and 2012-03-24...
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ab2ff3b5611f&tochange=df1f94b2bdee

Comment 3

[(mostly gone) XtC4UaLL [:xtc4uall]]

Thanks!

for Inbound this is:
Last good nightly: 2012-03-22
First bad nightly: 2012-03-23

Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=dd6e82e5edac&tochange=d2693e86769d

Comment 4

[(mostly gone) XtC4UaLL [:xtc4uall]]

HG Bisect tells:
The first bad revision is:
changeset:   89771:5b61e3d75735
user:        Michal Novotny <[e-mail]>
date:        Thu Mar 22 23:53:55 2012 +0100
summary:     Bug 722033 - Use asyncOpenCacheEntry() in nsHttpChannel::OpenOfflineCacheEntryForWriting()

(Had to apply add2ed43cc05 on each Run due to Build Failures)

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

cc'ing Michal on this - are we looking at a forward fix here or a backout of bug 722033 to make sure we don't ship 14 with this regression?

Comment 6

[Michal Novotny [:michal]]

Attachment: fix

We call ContinueConnect() in OnCacheEntryAvailableInternal() even if the channel was canceled. This patch is a quick fix for this issue. I still need to check all other failures in OnNormalCacheEntryAvailable(), OnOfflineCacheEntryAvailable() and OnOfflineCacheEntryForWritingAvailable() whether we correctly cancel the request or continue without the entry...

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 638761 [details] [diff] [review]
fix

Review of attachment 638761 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

checked this aborts the channel and calls OnStart/OnStopRequest just ones for this case

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5522,5 @@
>                  }
>                  return NS_ERROR_DOCUMENT_NOT_CACHED;
>              }
> +            if (mCanceled)
> +                return rv;

Definitely needs a comment what this causes and why it is here.

Comment 8

[Michal Novotny [:michal]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/dadb511cc04c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dadb511cc04c

Comment 10

[(mostly gone) XtC4UaLL [:xtc4uall]]

Verified using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120705030540. Thanks for the quickly Fix!

Comment 11

[Michal Novotny [:michal]]

Comment on attachment 638761 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 722033
User impact if declined: some resources would be fetched from the net multiple times
Testing completed (on m-c, etc.): https://tbpl.mozilla.org/?tree=Try&rev=dad6ce8b9a25
Risk to taking this patch (and alternatives if risky): probably none
String or UUID changes made by this patch: none

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 638761 [details] [diff] [review]
fix

[Triage Comment]
Approving for all branches in case the http://www.crazy-factory.com/ user experience is a canary in the coal mine for other web regressions caused by bug 722033.

Comment 13

[Michal Novotny [:michal]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ebea5096017e
https://hg.mozilla.org/releases/mozilla-beta/rev/cd89f913b87b

Comment 14

[Alex Keybl [:akeybl]]

(In reply to Michal Novotny (:michal) from comment #13)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/ebea5096017e
> https://hg.mozilla.org/releases/mozilla-beta/rev/cd89f913b87b

Thanks Michal!