Last Comment Bug 717759 - negative pipelining feedback on failed image parses
: negative pipelining feedback on failed image parses
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 11 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Patrick McManus [:mcmanus] PTO until Sep 6
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-12 14:26 PST by Patrick McManus [:mcmanus] PTO until Sep 6
Modified: 2012-03-23 05:58 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 0 (8.29 KB, patch)
2012-01-12 14:32 PST, Patrick McManus [:mcmanus] PTO until Sep 6
bzbarsky: review+
joe: review+
Details | Diff | Splinter Review
patch v1 (9.01 KB, patch)
2012-02-15 09:32 PST, Patrick McManus [:mcmanus] PTO until Sep 6
honzab.moz: review+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] PTO until Sep 6 2012-01-12 14:26:50 PST
this is a long planned item for pipelining "negative feedback". Correlated with the old pipelining failures were pages with a lot of broken images (occasionally js or css was getting fed into images) - for various reasons most of that has disappeared from the net, but its still a reasonable metric for whether pipelining is running ok - at least on the negative side.

Even if there are legit errors that means cancellations are going to happen, connections are going to close, and that's bad for pipelining too. Basically, pipelines are speculating on a future clean environment.

Consumer code that discovers a problem with data from necko can report its nsiuri through the new net:failed-to-process-uri observer event. Right now the only thing that makes that kind of report is imagelib - it seemed to me that parse errors are just too common in other kinds of resources. Right now necko just uses this as pipeline feedback, but you can imagine maybe invalidating caches feeding this information into a database as potential future uses of it.

there's even a test.
Comment 1 Patrick McManus [:mcmanus] PTO until Sep 6 2012-01-12 14:32:14 PST
Created attachment 588186 [details] [diff] [review]
patch 0
Comment 2 Boris Zbarsky [:bz] 2012-01-31 21:33:34 PST
Comment on attachment 588186 [details] [diff] [review]
patch 0

Looks fine to me, but could use an actual imagelib peer too.
Comment 3 Patrick McManus [:mcmanus] PTO until Sep 6 2012-02-01 17:11:15 PST
Comment on attachment 588186 [details] [diff] [review]
patch 0

joe, can you glance at the imagelib bit of this?
Comment 4 Joe Drew (not getting mail) 2012-02-01 19:03:39 PST
Comment on attachment 588186 [details] [diff] [review]
patch 0

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

One thing to note is that we're only guaranteed to look at the first N bytes of an image (N changes as of bug 715308), so you cannot rely on this to guarantee that images decode correctly. That is, onerror only fires if the file is absolutely not an image, not if it is subtly corrupt.

::: image/src/imgRequest.cpp
@@ +704,5 @@
> +
> +    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +    if (os)
> +      os->NotifyObservers(mURI, "net:failed-to-process-uri", nsnull);
> +  }

Please put this block above the comment block.

::: image/test/mochitest/test_net_failedtoprocess.html
@@ +30,5 @@
> +
> +  observe: function(subject, topic, data) {
> +    ok(topic == "net:failed-to-process-uri", "wrong topic");
> +    subject = subject.QueryInterface(Ci.nsIURI);
> +    ok(subject.asciiSpec == "chrome://mochitests/content/chrome/image/test/mochitest/invalid.jpg", "wong subject");

wrong, not wong

@@ +37,5 @@
> +};
> +
> +var obs = Cc["@mozilla.org/observer-service;1"].getService();
> +obs = obs.QueryInterface(Ci.nsIObserverService);
> +obs.addObserver(observer, "net:failed-to-process-uri", false);

I don't *think* this will work in a world where we need SpecialPowers to use chrome things. I'd prefer this test to be rewritten using SpecialPowers. (If this, in fact, is not just a chrome test; it's not obvious to me.)
Comment 5 Honza Bambas (:mayhemer) 2012-02-13 11:12:29 PST
Comment on attachment 588186 [details] [diff] [review]
patch 0

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

::: image/src/imgRequest.cpp
@@ +703,5 @@
> +    // Report the URI to net:failed-to-decode-uri observers.
> +
> +    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +    if (os)
> +      os->NotifyObservers(mURI, "net:failed-to-process-uri", nsnull);

Hmmm... shouldn't this topic be more specific like:
- "net:failed-image-content-processing-on-uri"
- "net:image-content-corrupted", or at least
- "net:failed-to-process-uri-content"

This seems to me too general and could potentially collide, but up to you.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +987,5 @@
> +    nsCAutoString host;
> +    PRInt32 port = -1;
> +    bool usingSSL = false;
> +
> +    nsresult rv = uri->SchemeIs("https", &usingSSL);

What if this is not http at all?

@@ +998,5 @@
> +
> +    nsRefPtr<nsHttpConnectionInfo> ci =
> +        new nsHttpConnectionInfo(host, port, nsnull, usingSSL);
> +    
> +    PipelineFeedbackInfo(ci, RedCorruptedContent, nsnull, 0);

Have an overload for connection entry instead of conn info?  We could have a static method to create the key from host,port,usingSSL,etc that nsHttpConnectionInfo can use and also this code can use.  Or, just find entry by nsIURI.

Creating nsHttpConnectionInfo for purpose of passing a hashing key doesn't seems right and necessary.

Both entries (anon/non-anon), if exist respectively, should get the feedback notification.
Comment 6 Patrick McManus [:mcmanus] PTO until Sep 6 2012-02-15 09:32:27 PST
Created attachment 597440 [details] [diff] [review]
patch v1
Comment 7 Honza Bambas (:mayhemer) 2012-02-15 09:53:57 PST
Comment on attachment 597440 [details] [diff] [review]
patch v1

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

r=honzab
Comment 8 Patrick McManus [:mcmanus] PTO until Sep 6 2012-03-20 10:38:45 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/672cc4ee2dd9
Comment 9 Matt Brubeck (:mbrubeck) 2012-03-20 12:59:48 PDT
Sorry, I backed this out on inbound because something in the push appeared to cause a leak in mochitest-browser-chrome:
https://hg.mozilla.org/integration/mozilla-inbound/rev/577da08878cb
Comment 10 Patrick McManus [:mcmanus] PTO until Sep 6 2012-03-22 18:04:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6018a0d6b33d
Comment 11 Marco Bonardo [::mak] 2012-03-23 05:58:32 PDT
https://hg.mozilla.org/mozilla-central/rev/6018a0d6b33d

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