"Harness status: OK" + failing test when running "fetch-event.https.html" test

RESOLVED FIXED in Firefox 44

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: noemi, Assigned: Ehsan)

Tracking

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox44 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Reporter

Description

4 years ago
Checked with 7/6 master build

Test run such as |./mach web-platform-tests _mozilla/service-workers/service-worker/fetch-event.https.html|

Result:
* Harness status: OK
* Found 9 tests
* 8 Pass
* 1 Fail:
** Service Worker responds to fetch event with POST form	
*** assert_unreached: unexpected rejection: assert_equals: expected "POST:testName1=testValue1&testName2=testValue2" but got "POST:Content-Type: application/x-www-form-urlencoded\nContent-Length: 41\n\ntestName1=testValue1&testName2=testValue2" Reached unreachable code
unreached_rejection/<@https://web-platform.test:8443/_mozilla/service-workers/service-worker/resources/test-helpers.sub.js:42:7 Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1363:20 Test.prototype.step_func/<@https://web-platform.test:8443/resources/testharness.js:1387:1 Promise*@https://web-platform.test:8443/_mozilla/service-workers/service-worker/fetch-event.https.html:125:5 Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1363:20 async_test@https://web-platform.test:8443/resources/testharness.js:513:13 @https://web-platform.test:8443/_mozilla/service-workers/service-worker/fetch-event.https.html:122:1 
* Traces: https://pastebin.mozilla.org/8838637
Reporter

Updated

4 years ago
Summary: "Harness status: OK" + failing test when running wpt "fetch-event.https.html" test → "Harness status: OK" + failing test when running "fetch-event.https.html" test
Reporter

Updated

4 years ago
Component: DOM: Service Workers → web-platform-tests
Product: Core → Testing
This test fails because forms use mime-input-stream internally to serialize form data. nsMIMEInputStream adds Content-Length header to the stream data when it is read. ServiceWorkerManager sticks this into Request.body. It should parse the headers and set them as headers. The clean way to do this seems to be:

1) Add a GetHeaders() to nsIMimeInputStream which reads out the headers stream. Add corresponding invariant checks to make sure this is reset when the stream is Seek()ed and so on.
2) Have ServiceWorkerManager check if it is a nsIMimeInputStream, if yes, read out the headers and then set the body as the request body.

Unfortunately this problem quickly degenerates if a multiplex stream wraps the mime input stream.

Any ideas Josh?
Flags: needinfo?(josh)
My suggestion on IRC was to add a canary mechanism to let us know if the multiplex stream occurs.
Flags: needinfo?(josh)
Assignee

Comment 3

4 years ago
Josh, can you please elaborate the details of the IRC conversation?  This seems like a pretty broken behavior to me, it would be better to fix it now.
Flags: needinfo?(josh)
The log is here: http://logs.glob.uno/?c=mozilla%23content&s=21+Sep+2015&e=21+Sep+2015&h=multiplex#c323291
I couldn't come up with an actual solution at the time besides banning it.
Flags: needinfo?(josh)
Assignee

Comment 5

4 years ago
Thanks for the link!

Here is an idea.  How about adding a new interface like this:

interface nsICompositeInputStream : nsISupports {
  void seekToMainBody();
}

And in the SW code, QI to nsICompositeInputStream and if it succeeds, call seekToMainBody() before reading the content.

Then, we'll make nsMIMEInputStream implement it and seek past the headers.  Furthermore, we'll make nsMultiplexInputStream implement it as well, and QI the wrapped streams to nsICompositeInputStream and call SeekToMainBody() on them.

Thoughts?
Flags: needinfo?(josh)
As long as we don't end up with an incorrect Content-Length value by doing so (since that's the point of nsMIMEInputStream), that sounds fine to me.
Flags: needinfo?(josh)
Assignee

Comment 7

4 years ago
Talked about this with Josh in person.  The test here is just wrong, we _are_ supposed to see the raw body data on the Request object.  We already have mochitests to that effect.
Assignee: nobody → ehsan
Assignee

Comment 8

4 years ago
Attachment #8676470 - Flags: review?(bkelly)
Comment on attachment 8676470 [details] [diff] [review]
Make fetch-event.https.html pass

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

::: testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-event.https.html
@@ +155,5 @@
>          })
>        .then(function(frame) {
>            assert_equals(frame.contentDocument.body.textContent,
> +                        'POST:Content-Type: application/x-www-form-urlencoded\n' +
> +                        'Content-Length: 41\n\n' +

This seems wrong to me.  The service worker does:

  event.respondWith(new Promise(function(resolve) {
      event.request.text()
        .then(function(result) {
            resolve(new Response(event.request.method + ':' + result));
          });
    }));

Why are the form post headers included in event.request.text()?
Attachment #8676470 - Flags: review?(bkelly) → review-
Assignee

Comment 10

4 years ago
These are not headers, they're part of the multipart/form-data encoded body of the request.  The SW gets the full body of the request and tests against that, so not giving it the full content of the multipart/form-data encoded body is a mistake.
Assignee

Updated

4 years ago
Attachment #8676470 - Flags: review- → review?(bkelly)
So, I'm having a hard time finding something that says the Content-Length is required.  Can you point me at the spec that requires that?  I'm just wondering if this condition is too gecko specific.
Flags: needinfo?(ehsan)
Assignee

Comment 12

4 years ago
I'm not sure which spec Content-Length comes from.  Boris, do you know?
Flags: needinfo?(ehsan) → needinfo?(bzbarsky)
I mean, maybe we should do an indexOf() instead of an exact equality match.
The relevant spec here is a combination of the HTML spec and RFC 2388.  Except the latter doesn't matter since this test doesn't have any multipart/form-data going on anywhere.  It's a application/x-www-form-urlencoded POST, because there is no enctype set on the form.  So comment 10 is wrong.

So the entrypoint is https://html.spec.whatwg.org/multipage/forms.html#form-submission-algorithm which in this case leads to https://html.spec.whatwg.org/multipage/forms.html#submit-body which lands us in <https://html.spec.whatwg.org/multipage/forms.html#application/x-www-form-urlencoded-encoding-algorithm>.  There are no internal headers in this body.

The "Content-Length: 41" is precisely the length of the "testValue1=testName1&testValue2=testName2" string that we're sending as the entity body.

The fundamental problem you're running into is the necko allows providing a "post data stream" that includes some headers.  For the HTTP impl, that's fine: it just dumps it all on the wire in order; it just needs to be told to not add its own "\r\n\r\n" before the data stream, and the data stream needs to provide that after its headers.

So the earlier discussion about having ways to ask such data streams for their headers and data separately seems right to me.
Flags: needinfo?(bzbarsky)
Yea, it seems we need to have ServiceWorkerPrivate::DispatchFetchEvent() take into account the HttpBaseChannel::mUploadStreamHasHeaders value.

  https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#744

I think maybe should leave this one as expected fail for now and try to fix after v1.
Assignee

Comment 16

4 years ago
Ah, seems like I ended up fighting the windmills here.  :(
Attachment #8676470 - Flags: review?(bkelly) → review-
Assignee

Comment 17

4 years ago
These two classes were not moved at the same time their consumer
moved from Fetch.cpp to FetchUtil.cpp.
Attachment #8678515 - Flags: review?(bkelly)
Assignee

Updated

4 years ago
Attachment #8676470 - Attachment is obsolete: true
Assignee

Comment 20

4 years ago
Attachment #8678518 - Flags: review?(bkelly)
Attachment #8678515 - Flags: review?(bkelly) → review+
Attachment #8678516 - Flags: review?(bkelly) → review+
Comment on attachment 8678517 [details] [diff] [review]
Part 3: Correctly handle upload channels that have embedded body headers when dispatching a FetchEvent

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

r=me with comment addressed.

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +948,1 @@
>          rv = uploadChannel->CloneUploadStream(getter_AddRefs(mUploadStream));

I don't think we want to set mUploadStream here.  If any of the checks in HandleBodyWithHeaders() fails below then we leave the stream-with-headers set as mUploadStream.

I think we should read this to a temporary, pass it in to HandleBodyWithHeaders(), and then assign the final result to mUploadStream.

@@ +1099,5 @@
> +  {
> +    // We are dealing with an nsMIMEInputStream which uses string input streams
> +    // under the hood, so all of the data is available synchronously.
> +    nsAutoCString body;
> +    nsresult rv = NS_ConsumeStream(mUploadStream, UINT32_MAX, body);

This is a bit scary.  It depends on details buried multiple layers down in different stream implementations.

Please check for non-blocking and return failure if the stream is blocking.

Also, please get the Available() bytes and only consume that many.

@@ +1110,5 @@
> +    const nsAutoCString::const_iterator body_end = end;
> +    nsAutoCString headerName, headerValue;
> +    bool emptyHeader = false;
> +    while (FetchUtil::ExtractHeader(begin, end, headerName,
> +                                    headerValue, &emptyHeader) &&

This works for now, but I can't help but wonder if it would be easier to expose methods on nsIMIMEInputStream to get the headers and body streams separately.  They would only work if the stream had not been accessed yet (and thus not drained to the multiplex stream internally).

@@ +1123,5 @@
> +    nsCOMPtr<nsIStringInputStream> strStream =
> +      do_CreateInstance(NS_STRINGINPUTSTREAM_CONTRACTID, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    ++begin;
> +    ++begin;

This is skipping the \n\r?  Can you write a comment to that effect?

@@ +1124,5 @@
> +      do_CreateInstance(NS_STRINGINPUTSTREAM_CONTRACTID, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    ++begin;
> +    ++begin;
> +    body.Assign(Substring(begin, body_end));

Can this be done with a dependent string?  Or possibly just passing the Substring(begin, body_end) directly to strStream->SetData()?

It just seems we might be making an extra copy here.
Attachment #8678517 - Flags: review?(bkelly) → review+
Comment on attachment 8678518 [details] [diff] [review]
Part 4: Make fetch-event.https.html pass

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

Thanks!
Attachment #8678518 - Flags: review?(bkelly) → review+
Assignee

Comment 23

4 years ago
(In reply to Ben Kelly [:bkelly] from comment #21)
> ::: dom/workers/ServiceWorkerPrivate.cpp
> @@ +948,1 @@
> >          rv = uploadChannel->CloneUploadStream(getter_AddRefs(mUploadStream));
> 
> I don't think we want to set mUploadStream here.  If any of the checks in
> HandleBodyWithHeaders() fails below then we leave the stream-with-headers
> set as mUploadStream.
> 
> I think we should read this to a temporary, pass it in to
> HandleBodyWithHeaders(), and then assign the final result to mUploadStream.

Good idea!

> @@ +1099,5 @@
> > +  {
> > +    // We are dealing with an nsMIMEInputStream which uses string input streams
> > +    // under the hood, so all of the data is available synchronously.
> > +    nsAutoCString body;
> > +    nsresult rv = NS_ConsumeStream(mUploadStream, UINT32_MAX, body);
> 
> This is a bit scary.  It depends on details buried multiple layers down in
> different stream implementations.
> 
> Please check for non-blocking and return failure if the stream is blocking.

Fair enough.

> Also, please get the Available() bytes and only consume that many.

That's what NS_ConsumeStream does: <https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsStreamUtils.cpp?case=true&from=NS_ConsumeStream#655> (it reads all of the available bytes.)

> @@ +1110,5 @@
> > +    const nsAutoCString::const_iterator body_end = end;
> > +    nsAutoCString headerName, headerValue;
> > +    bool emptyHeader = false;
> > +    while (FetchUtil::ExtractHeader(begin, end, headerName,
> > +                                    headerValue, &emptyHeader) &&
> 
> This works for now, but I can't help but wonder if it would be easier to
> expose methods on nsIMIMEInputStream to get the headers and body streams
> separately.  They would only work if the stream had not been accessed yet
> (and thus not drained to the multiplex stream internally).

That's what I explored doing originally (see comment 5) but now I think this is actually simpler than the alternative, so I personally don't think we need to do anything further in the future.  At least this code is robust if for example some day we change whether we're using a multiplex stream for this.  :-)

> @@ +1123,5 @@
> > +    nsCOMPtr<nsIStringInputStream> strStream =
> > +      do_CreateInstance(NS_STRINGINPUTSTREAM_CONTRACTID, &rv);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    ++begin;
> > +    ++begin;
> 
> This is skipping the \n\r?  Can you write a comment to that effect?

Yes, will do.

> @@ +1124,5 @@
> > +      do_CreateInstance(NS_STRINGINPUTSTREAM_CONTRACTID, &rv);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    ++begin;
> > +    ++begin;
> > +    body.Assign(Substring(begin, body_end));
> 
> Can this be done with a dependent string?  Or possibly just passing the
> Substring(begin, body_end) directly to strStream->SetData()?
> 
> It just seems we might be making an extra copy here.

There's no extra copy here.  Substring() returns a dependent string so that the .Assign() call ends up not copying the data: <https://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsTSubstring.cpp#411>.  SetData() of course copies the string but that's unavoidable.
You need to log in before you can comment on or make changes to this bug.