Open Bug 1155431 Opened 10 years ago Updated 2 years ago

Image with HTTP 204 response triggers two requests

Categories

(Core :: Graphics: ImageLib, defect)

37 Branch
x86_64
Linux
defect

Tracking

()

People

(Reporter: ricardo, Unassigned, Mentored)

References

()

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files, 1 obsolete file)

Attached image Firefox Network tab
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/41.0.2272.76 Chrome/41.0.2272.76 Safari/537.36 Steps to reproduce: 1. Open this page https://code.google.com/p/chromium/issues/detail?id=477843 2. Open developer tools, network tab (or use Wireshark) 3. Look for requests for pixel.php Actual results: Two consecutive (non-parallel) requests for pixel.php are triggered. Expected results: Only one request should have happened.
Link to Chromium bug report: https://code.google.com/p/chromium/issues/detail?id=477843 Seems to happen only in Chrome/Chromium and Firefox. pixel.php code: <?php header('Content-Type: image/gif'); if (isset($_GET['image'])) { echo base64_decode('R0lGODlhAQABAIAAAAUEBAAAACwAAAAAAQABAAACAkQBADs='); } else { http_response_code(204); } empty.js is really empty, can be loaded without ssl http://test.gumgum.com/script-image-204-bug/ can receive the following: -?nospan = removes the span element (in FF this fixes it) -?noscript = removes the script (fixes for FF and Ch) -?nofake = removes the <img pixel.php> (shows bug only applies to 204 image) -?noreal = removes the <img pixel.gif> -?image = forces pixel.php to return a black pixel (fixes both)
bump this up to imglib.. networking seems to work ok for pixel.php.. it generates OnStartRequest(NS_OK.. response headers with content type) and then OnStopRequest(NS_OK) which corresponds to what the server said. There are no OnDataAvailable() calls because it is a 0 byte response. I'm not sure why imglib responds to this by trying again.
Component: Networking: HTTP → ImageLib
Status: UNCONFIRMED → NEW
Ever confirmed: true
Without debugging this at all I'm guessing this is the parser speculatively calling nsDocument::MaybePreLoadImage for the image it encounters while it it waiting for the script to load.
see also https://bugzilla.mozilla.org/show_bug.cgi?id=685740 (I don't have a STR but I know I've seen multiple requests for favicon when it isn't provided)
Whiteboard: [gfx-noted]
Assignee: nobody → mvolmer
Mentor: seth
Usually we don't send duplicate requests because the second request gets a hit in the image cache, which is defined in |image/imgLoader.h| and |image/imgLoader.cpp|. It'd be good to know why that isn't happening here. I suspect the bug is in some combination of imgLoader and imgRequest. I don't think we currently have test coverage for the case where we don't get even a single OnDataAvailable call (which comment 2 suggests it probably happening here). Those two classes are the first places I'd look.
Comment on attachment 8631809 [details] [diff] [review] Cancel the request only if it does not succeed Review of attachment 8631809 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for posting your patch, Mihai! I don't think this is quite what we want yet, but we're almost there! BTW, you need to have the reviewer listed in your commit message - use "r=<irc nick>" for this. So in this case, the commit message should be "Bug 1155431: Cancel the request only if it does not succeed. r=seth". ::: image/imgRequest.cpp @@ +868,5 @@ > // if the error isn't "just" a partial transfer > // stops animations, removes from cache > + if (!NS_SUCCEEDED(status)) { > + this->Cancel(status); > + } I don't think this is the approach we want to take, because we're still setting mImageErrorCode in this branch. If we make this change, we'll be introducing a new state that didn't exist before, where an image could have an error code set and yet not be cancelled. Probably instead, you should either: (1) Move the |!NS_SUCCEEDED(status)| check onto the |else| clause, ending up with |else if (!NS_SUCCEEDED(status))|, or... (2) Remove the |image &&| check from the |if| clause, and add an |if (image)| check around the call to UpdateCacheEntrySize(), since UpdateCacheEntrySize() requires a non-null image| value.
Attachment #8631809 - Flags: review?(seth)
Comment on attachment 8631893 [details] [diff] [review] Cancel the request only if it does not succeed. Fixed mImageErrorCode being set for requests that are not canceled. Review of attachment 8631893 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK. Let's see how it does on try.
Attachment #8631893 - Flags: review?(seth) → review+
NS_FAILED instead of !NS_SUCCEEDED, perhaps?

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Assignee: mihaivolmer → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: