Noticed while debugging, prob not breaking anything serious now, but ought to be fixed. If an HTTP reply lacks Content-Type info we set up a nsIStreamConverterService to sniff the data. It delays calling the real listener's OnStartRequest/OnData/OnStop, possibly until the regular channel OnStop is done. This means that when the real listener's OnStart gets called, the channel's mIsPending has already been set to false. It should be true until just before the listener's onStop is called. STR: write an xpcshell test that replies w/o Content-Type, add a check to head_channel.js to check isPending() in OnStartReq (have patch that does it among lots of other stuff: don't want to split out here).

Now that we added the "ForcePending()" call in bug 975338 it should be fairly easy to fix this--we just need to force pending on while we deliver OnStart/OnData.

Comment on attachment 8437494 [details] [diff] [review] bug-748117-fix.patch Review of attachment 8437494 [details] [diff] [review]: ----------------------------------------------------------------- Very close to ready! A few minor issues... One thing your patch is missing: take the implementation of nsHttpChannel::IsPending() (which checks for mForcePending) and move that logic to HttpBaseChannel::IsPending. Then you can get rid of the nsHttpChannel version since the base implementation will be the same. Normally I'd just mark the patch +r and say you can land it with the fixes, but since this is your first patch let me actually review the next one again. ::: netwerk/protocol/http/HttpBaseChannel.h @@ +412,5 @@ > ("HttpAsyncAborter::AsyncAbort [this=%p status=%x]\n", mThis, status)); > > mThis->mStatus = status; > + //mThis->mIsPending equals false need to be set in HandleAsyncAbort function > + //because in some cases listener need to be notified True--so just get rid of the commented-out lines. ::: netwerk/protocol/http/HttpChannelChild.cpp @@ +711,5 @@ > LOG(("HttpChannelChild::FailedAsyncOpen [this=%p status=%x]\n", this, status)); > > mStatus = status; > +// mIsPending = false will be set inside HandleAsyncAbort function > + remove this comment too. ::: netwerk/protocol/http/nsIHttpChannelInternal.idl @@ +208,5 @@ > + * Or, > + * it is used when connection is very short and content-type is not set. So > + * content sniffer delays calling > + * OnStartRequest/OnDataAvailable/OnStopRequest > + * and OnStopRequest of the real channel is called first. Let's keep it short: /** * ForcePending(true) overrides the normal behavior for the * channel's IsPending(), forcing 'true' to be returned. A call to * ForcePending(false) reverts IsPending() back to normal behavior. */ We usually avoid too much discussion in comments of what code calls an API, especially once it's called by more than one thing--otherwise the comments tend to get inaccurate as code changes. ::: netwerk/streamconv/converters/nsUnknownDecoder.cpp @@ +199,5 @@ > // > if (mContentType.IsEmpty()) { > DetermineContentType(request); > > + // For a very short connection without a conntent-type, the following function (FireListenerNotification) s/conntent/content/ @@ +201,5 @@ > DetermineContentType(request); > > + // For a very short connection without a conntent-type, the following function (FireListenerNotification) > + // sends OnStartRequest to the listener, but the channel have already received OnStopRequest and therefore > + // it is not in pending state. We need to force pending state. maybe the whole comment can just be // Make sure channel listeners see channel as pending while we call // OnStartRequest/OnDataAvailable, even though the underlying channel // has already hit OnStopRequest. ::: netwerk/test/unit/head_channels.js @@ +84,5 @@ > if (!(this._flags & (CL_EXPECT_FAILURE | CL_ALLOW_UNKNOWN_CL))) > do_throw("Could not get contentLength"); > } > + if (!request.isPending()) > + do_throw("request reports itself as not pending from onStartRequest!"); Also add this same check to OnDataAvailable() ::: netwerk/test/unit/test_reply_without_content_type.js @@ +1,2 @@ > +// > +// Simple HTTP test: fetches page Change to // tests HTTP replies that lack content-type (where we try to sniff content-type). @@ +7,5 @@ > +Cu.import("resource://testing-common/httpd.js"); > + > +var httpserver = new HttpServer(); > +var testpath = "/simple"; > +var httpbody = "0123456789"; While we're adding a test for this, it might make sense to actually test the sniffer's correctness (I don't know if we have any tests for it yet). So try changing the httpbody to some HTML, for instance, and let's test in CheckRequest that the nsIChannel.contentType is html. I'm not actually sure what heuristics it used--probably just "<html><body>omg hai</body></html>" is good enough.

> @@ +84,5 @@ > > if (!(this._flags & (CL_EXPECT_FAILURE | CL_ALLOW_UNKNOWN_CL))) > > do_throw("Could not get contentLength"); > > } > > + if (!request.isPending()) > > + do_throw("request reports itself as not pending from onStartRequest!"); > > Also add this same check to OnDataAvailable() Ths was already there.

Comment on attachment 8443268 [details] [diff] [review] Fix v3 Review of attachment 8443268 [details] [diff] [review]: ----------------------------------------------------------------- Looks ready to land! \o/ One nit for future reference: when you submit patches to a bug, consider labeling them "v1", "v2", etc, so it's clearer which is which when people do things like try to diff one against another. (If the patch has multiple parts I usually do something like "Part1, v1: remove nsIFoo interface"). ::: netwerk/test/unit_ipc/test_reply_without_content_type_wrap.js @@ +2,5 @@ > +// Run test script in content process instead of chrome (xpcshell's default) > +// > + > +function run_test() { > + run_test_in_child("../unit/test_reply_without_content_type_wrap.js"); Thanks for adding the e10s test!

Comment on attachment 8437494 [details] [diff] [review] bug-748117-fix.patch Oh, and it's good to hide obsolete patches in a bug (this was my first cut at a fix). To do that is really obscure: hit "details", then "Edit Details" at the top right, and on the left there's a "obsolete" field (you can also mark something a "patch" if someone uploaded one w/o marking it as a patch). Ah, Bugzilla... :)

Can we please run this through Try first? :)

just fixed e10s test.

Comment on attachment 8446472 [details] [diff] [review] Fix v5 Review of attachment 8446472 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Have you run this through tryserver yet? (people often just paste the link to the try run into the bug to show that).