IsPending broken for requests without Content-Type

RESOLVED FIXED in mozilla33

Status

()

Core
Networking: HTTP
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jduell, Assigned: dragana)

Tracking

unspecified
mozilla33
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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).
(Reporter)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
Created attachment 8436931 [details] [diff] [review]
bug-748117-fix.patch
Attachment #8436931 - Flags: review+
(Assignee)

Comment 3

4 years ago
Created attachment 8436984 [details] [diff] [review]
bug-748117-fix.patch
Attachment #8436931 - Attachment is obsolete: true
Attachment #8436984 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8436984 - Flags: review+ → review?(jduell.mcbugs)
(Assignee)

Comment 4

4 years ago
Created attachment 8437494 [details] [diff] [review]
bug-748117-fix.patch
Attachment #8436984 - Attachment is obsolete: true
Attachment #8436984 - Flags: review?(jduell.mcbugs)
Attachment #8437494 - Flags: review?(jduell.mcbugs)
(Reporter)

Comment 5

4 years ago
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.
Attachment #8437494 - Flags: review?(jduell.mcbugs) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 8443268 [details] [diff] [review]
Fix v3

> @@ +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.
Attachment #8443268 - Flags: review?(jduell.mcbugs)
(Reporter)

Comment 7

4 years ago
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!
Attachment #8443268 - Flags: review?(jduell.mcbugs) → review+
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
(Reporter)

Comment 8

4 years ago
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... :)
Attachment #8437494 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #8443268 - Attachment description: text/plain → Fix v3
Can we please run this through Try first? :)
Keywords: checkin-needed
(Assignee)

Comment 10

4 years ago
Created attachment 8445162 [details] [diff] [review]
Fix v4

just fixed e10s test.
Attachment #8443268 - Attachment is obsolete: true
Attachment #8445162 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

4 years ago
Attachment #8445162 - Attachment is obsolete: true
Attachment #8445162 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 11

4 years ago
Created attachment 8446472 [details] [diff] [review]
Fix v5
Attachment #8446472 - Flags: review?(jduell.mcbugs)
(Reporter)

Comment 12

4 years ago
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).
Attachment #8446472 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/37dc4a16c312
Assignee: nobody → dd.mozilla
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1061663
You need to log in before you can comment on or make changes to this bug.