Last Comment Bug 764337 - Firefox 14+ requests images inserted by JS multiple times if not cached
: Firefox 14+ requests images inserted by JS multiple times if not cached
: regression
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 14 Branch
: x86_64 Windows 7
-- normal (vote)
: mozilla16
Assigned To: Michal Novotny (:michal)
: Patrick McManus [:mcmanus]
Depends on:
Blocks: 722033 761534
  Show dependency treegraph
Reported: 2012-06-13 04:48 PDT by strayer
Modified: 2012-07-18 00:55 PDT (History)
10 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Fiddler2 Screenshot showing multiple requests (10.54 KB, image/png)
2012-06-13 04:48 PDT, strayer
no flags Details
fix (1.16 KB, patch)
2012-07-03 09:08 PDT, Michal Novotny (:michal)
honzab.moz: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image strayer 2012-06-13 04:48:39 PDT
Created attachment 632640 [details]
Fiddler2 Screenshot showing multiple requests

I visited 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 User image Boris Zbarsky [:bz] (still a bit busy) 2012-06-13 09:14:34 PDT
Steve, would you be willing to find the day that this broke using ?

I can totally understand if you don't want to commit to that, by the way.  Just let me know.
Comment 2 User image strayer 2012-07-01 07:07:57 PDT
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...
Comment 3 User image (mostly gone) XtC4UaLL [:xtc4uall] 2012-07-01 08:38:49 PDT

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

Comment 4 User image (mostly gone) XtC4UaLL [:xtc4uall] 2012-07-01 17:01:02 PDT
HG Bisect tells:
The first bad revision is:
changeset:   89771:5b61e3d75735
user:        Michal Novotny <>
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 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-07-02 11:39:02 PDT
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 User image Michal Novotny (:michal) 2012-07-03 09:08:56 PDT
Created attachment 638761 [details] [diff] [review]

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 User image Honza Bambas (:mayhemer) 2012-07-03 09:46:08 PDT
Comment on attachment 638761 [details] [diff] [review]

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


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 User image Michal Novotny (:michal) 2012-07-03 10:30:48 PDT
Comment 9 User image Ryan VanderMeulen [:RyanVM] 2012-07-03 16:07:18 PDT
Comment 10 User image (mostly gone) XtC4UaLL [:xtc4uall] 2012-07-05 12:58:04 PDT
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 User image Michal Novotny (:michal) 2012-07-05 13:03:34 PDT
Comment on attachment 638761 [details] [diff] [review]

[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.):
Risk to taking this patch (and alternatives if risky): probably none
String or UUID changes made by this patch: none
Comment 12 User image Alex Keybl [:akeybl] 2012-07-05 17:02:35 PDT
Comment on attachment 638761 [details] [diff] [review]

[Triage Comment]
Approving for all branches in case the user experience is a canary in the coal mine for other web regressions caused by bug 722033.
Comment 14 User image Alex Keybl [:akeybl] 2012-07-09 13:20:29 PDT
(In reply to Michal Novotny (:michal) from comment #13)

Thanks Michal!

Note You need to log in before you can comment on or make changes to this bug.