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

Status: RESOLVED FIXED

(Testing :: web-platform-tests, defect)

Description

[Noemí Freire (:noemi)]

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

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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?

Comment 2

[Josh Matthews [:jdm]]

My suggestion on IRC was to add a canary mechanism to let us know if the multiplex stream occurs.

Comment 3

[(no longer active)]

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.

Comment 4

[Josh Matthews [:jdm]]

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.

Comment 5

[(no longer active)]

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?

Comment 6

[Josh Matthews [:jdm]]

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.

Comment 7

[(no longer active)]

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.

Comment 8

[(no longer active)]

Attachment: Make fetch-event.https.html pass

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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()?

Comment 10

[(no longer active)]

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.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 12

[(no longer active)]

I'm not sure which spec Content-Length comes from.  Boris, do you know?

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

I mean, maybe we should do an indexOf() instead of an exact equality match.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 16

[(no longer active)]

Ah, seems like I ended up fighting the windmills here.  :(

Comment 17

[(no longer active)]

Attachment: Part 1: Move FormFillIterator and FormDataParser to FetchUtil.cpp

These two classes were not moved at the same time their consumer
moved from Fetch.cpp to FetchUtil.cpp.

Comment 18

[(no longer active)]

Attachment: Part 2: Refactor the code to extract an HTTP header from a buffer from FormDataParser

Comment 19

[(no longer active)]

Attachment: Part 3: Correctly handle upload channels that have embedded body headers when dispatching a FetchEvent

Comment 20

[(no longer active)]

Attachment: Part 4: Make fetch-event.https.html pass

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8678518 [details] [diff] [review]
Part 4: Make fetch-event.https.html pass

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

Thanks!

Comment 23

[(no longer active)]

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

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bcf816eb5ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/370b6e4c66a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ca8546e3cc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d81779015b8

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3bcf816eb5ec
https://hg.mozilla.org/mozilla-central/rev/370b6e4c66a6
https://hg.mozilla.org/mozilla-central/rev/3ca8546e3cc5
https://hg.mozilla.org/mozilla-central/rev/8d81779015b8