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
Status: VERIFIED FIXED
: 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)
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
verified


Attachments
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 strayer 2012-06-13 04:48:39 PDT
Created attachment 632640 [details]
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 [:bz] 2012-06-13 09:14:34 PDT
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 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...
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ab2ff3b5611f&tochange=df1f94b2bdee
Comment 3 (mostly gone) XtC4UaLL [:xtc4uall] 2012-07-01 08:38:49 PDT
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] 2012-07-01 17:01:02 PDT
HG Bisect tells:
The first bad revision is:
changeset:   89771:5b61e3d75735
user:        Michal Novotny <michal.novotny@gmail.com>
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 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 Michal Novotny (:michal) 2012-07-03 09:08:56 PDT
Created attachment 638761 [details] [diff] [review]
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) 2012-07-03 09:46:08 PDT
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) 2012-07-03 10:30:48 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/dadb511cc04c
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-07-03 16:07:18 PDT
https://hg.mozilla.org/mozilla-central/rev/dadb511cc04c
Comment 10 (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 Michal Novotny (:michal) 2012-07-05 13:03:34 PDT
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] 2012-07-05 17:02:35 PDT
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 14 Alex Keybl [:akeybl] 2012-07-09 13:20:29 PDT
(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!

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