Open
Bug 1155431
Opened 10 years ago
Updated 2 years ago
Image with HTTP 204 response triggers two requests
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
NEW
People
(Reporter: ricardo, Unassigned, Mentored)
References
()
Details
(Whiteboard: [gfx-noted])
Attachments
(2 files, 1 obsolete file)
40.92 KB,
image/png
|
Details | |
994 bytes,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 1•10 years ago
|
||
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)
Comment 2•10 years ago
|
||
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
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•10 years ago
|
||
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.
Comment 4•10 years ago
|
||
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)
Updated•10 years ago
|
Assignee: nobody → mvolmer
Mentor: seth
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
Attachment #8631809 -
Flags: review?(seth)
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
Attachment #8631809 -
Attachment is obsolete: true
Attachment #8631893 -
Flags: review?(seth)
Comment 9•10 years ago
|
||
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+
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
NS_FAILED instead of !NS_SUCCEEDED, perhaps?
Comment 12•3 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.
Assignee: mihaivolmer → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•