1278275 - Headers.get should incorporate getAll behaviour

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Josh Matthews [:jdm]]

It appears our implementation of the Headers interface is out of date with the specification. We have an extra getAll method which isn't present upstream, and our get method returns the first header that matches, rather than the combined value of all matching headers.

Specification:
https://fetch.spec.whatwg.org/#dom-headers-get

Code: dom/webidl/Headers.webidl, dom/fetch/Headers.h, dom/fetch/InternalHeaders.cpp

Tests:
./mach mochitest dom/tests/mochitest/fetch/
* this directory contains tests that will need to be updated
./mach web-platform testing/web-platform/fetch/api/headers/
* these tests should show unexpected results after changing the code; the results will need to be updated in testing/web-platform/meta/fetch/api/headers/

Comment 1

[Saurabh Singhal [:saurabhs]]

Hi, I would like to tackle this bug. Need some info about what the Headers interface is all about. Also, how can I get the source code?

Thanks.

Comment 2

[Josh Matthews [:jdm]]

You'll need to follow the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build for getting the code and compiling it. https://developer.mozilla.org/en-US/docs/Web/API/Headers should be able to explain what you want to know about the Headers interface.

Comment 3

[Saurabh Singhal [:saurabhs]]

Thanks Josh. I will start now.

Comment 4

[Jinank Jain[:jj]]

Hi Josh, So are you planning to completely remove GetAll function because I tried doing that and it creates lots of other issues so I was thinking that we could keep the GetAll function and just change get definition, which says all the name values must be separated by commas.

Comment 5

[Jinank Jain[:jj]]

Attachment: patch.diff

This patch fixes the functionality of get Function, now we are appending all the fetched values to result.

Comment 6

[Saurabh Singhal [:saurabhs]]

I am already working on this issue and I have got the code to compile after removing the GetAll method everywhere it is accessed. Tests still need to be modified.

Comment 7

[Josh Matthews [:jdm]]

Comment on attachment 8773840 [details] [diff] [review]
patch.diff

I'm clearing review on this for the time being, since Saurabh was working on this already.

Comment 8

[Jinank Jain[:jj]]

(In reply to Josh Matthews [:jdm] from comment #7)
> Comment on attachment 8773840 [details] [diff] [review]
> patch.diff
> 
> I'm clearing review on this for the time being, since Saurabh was working on
> this already.

yeah sure I submitted a patch as I saw no activity on this bug from long time. As Saurabh he is working on it still I could delete my patch.

Comment 9

[Saurabh Singhal [:saurabhs]]

Hi Josh, on running
./mach mochitest dom/tests/mochitest/fetch/test_fetch_app_protocol.html

I am getting TypeError: navigator.mozApps is undefined

I tried reading up on mozApps but couldn't find anything related to the problem. How can I solve this issue? Also, how is navigator.mozApps defined for the test environment?

Comment 10

[Josh Matthews [:jdm]]

You can ignore that failures - it is automatically skipped in non-Firefox OS build environments (since navigator.mozApps is a Firefox OS API). Update the test as best you can, and we'll run it against our CI system to ensure that it passes.

Comment 11

[Josh Matthews [:jdm]]

Oh, and in the sharing how I knew this, I looked at mochitest.ini in the same directory and read the `skip-if` conditions for that particular test file.

Comment 12

[Saurabh Singhal [:saurabhs]]

Attachment: HeadersgetAll.patch

Comment 13

[Josh Matthews [:jdm]]

Comment on attachment 8780790 [details] [diff] [review]
HeadersgetAll.patch

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

Great work! I'd like to see a new patch that includes the API suggestions I've made; I think it will reduce the number of changes that are required. Please also add a commit messages like "Bug 1278275 - Update Headers::GetAll and Headers::Get to match the specification. r=jdm".

::: dom/cache/AutoUtils.cpp
@@ +263,5 @@
>  
>          ErrorResult headerRv;
>          nsAutoCString value;
> +        nsTArray<nsCString> values;
> +        requestHeaders->GetAll(header, values, headerRv);

Let's add a GetFirst C++ API so we don't duplicate this pattern in many places. That should let us get rid of GetAll, too.

::: dom/fetch/FetchDriver.cpp
@@ +339,5 @@
>      mRequest->GetBody(getter_AddRefs(bodyStream));
>      if (bodyStream) {
>        nsAutoCString method;
>        mRequest->GetMethod(method);
> +      nsAutoCString aValue;

The `a` prefix is used for arguments; this should be called contentTypeValue.

::: dom/fetch/InternalHeaders.cpp
@@ +86,5 @@
>    if (IsInvalidName(lowerName, aRv)) {
>      return;
>    }
>  
> +  const char* delimiter = ", ";

The specification says there should be no space after ,: https://fetch.spec.whatwg.org/#concept-header-value-combined

@@ +93,3 @@
>    for (uint32_t i = 0; i < mList.Length(); ++i) {
>      if (lowerName == mList[i].mName) {
> +      if( firstValueFound ) {

nit: use `if (firstValueFound) {`

@@ +100,5 @@
>      }
>    }
>  
>    // No value found, so return null to content
> +  if(aValue.IsEmpty()) {

nit: space after if

@@ +363,4 @@
>    MOZ_ASSERT(!result.Failed());
>  
>    AutoTArray<nsCString, 5> exposeNamesArray;
> +  nsAutoCString aValue;

This should be called exposedNamesValue.

::: testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini
@@ +5,3 @@
>  
>    [Check append methods when called with already used name]
> +    expected: PASS 

This file can be removed now; the default state is pass for all tests if no ini file is present.

Comment 14

[Saurabh Singhal [:saurabhs]]

Thanks for the review Josh. I will fix the outstanding issues soon.

Comment 15

[Saurabh Singhal [:saurabhs]]

(In reply to Josh Matthews [:jdm] (away until 9/3) from comment #13)
> Let's add a GetFirst C++ API so we don't duplicate this pattern in many
> places. That should let us get rid of GetAll, too.

This piece of code uses GetAll:
dom/cache/AutoUtils.cpp:
    AutoTArray<nsCString, 16> varyHeaders;
    ErrorResult rv;
    cachedResponseHeaders->GetAll(NS_LITERAL_CSTRING("vary"), varyHeaders, rv);
    MOZ_ALWAYS_TRUE(!rv.Failed());

    // Assume the vary headers match until we find a conflict
    bool varyHeadersMatch = true;

    for (uint32_t j = 0; j < varyHeaders.Length(); ++j) {

I don't think we can get rid of GetAll.

Comment 16

[Saurabh Singhal [:saurabhs]]

Attachment: HeadersgetAll.patch

Addressing review comments from jdm@

Comment 18

[Saurabh Singhal [:saurabhs]]

Hey Josh, is there anything else that needs to be fixed in the latest patch?

Comment 19

[Josh Matthews [:jdm]]

Comment on attachment 8786799 [details] [diff] [review]
HeadersgetAll.patch

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

Looks great! Sorry about the delay; I was on vacation last week and Monday was a Canadian holiday.

Comment 20

[Josh Matthews [:jdm]]

Saurabh, do you have access to the tryserver to run the automated testsuite against these changes? If not, please follow the steps at https://www.mozilla.org/hacking/committer/ and I'll vouch for you.

Comment 21

[Saurabh Singhal [:saurabhs]]

I don't know about tryserver. I will follow the link you have given.

Comment 22

[Saurabh Singhal [:saurabhs]]

I have filed BUG1301685 for commit access to the tryserver.

Comment 25

[Saurabh Singhal [:saurabhs]]

Pushed the changes to the try server: ./mach try -b d -p all -u saurabh dom/tests/ testing/web-platform/tests/dom/

Comment 26

[Saurabh Singhal [:saurabhs]]

Results are here : https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5dd2d6069c5

Comment 27

[Josh Matthews [:jdm]]

Unfortunately, `./mach try` doesn't understand directories like that, I believe. We ended up only testing subsets of a bunch of different test suites. Let's use the following syntax instead (obtained from selecting all the mochitest-N, e10s-mochitest-N, web-platform-test-N, and e10s-web-platform-test-N entries):
./mach try -b do -p linux,linux64,macosx64,win32,win64 -u mochitest-1,mochitest-2,mochitest-3,mochitest-4,mochitest-5,mochitest-e10s-1,mochitest-e10s-2,mochitest-e10s-3,mochitest-e10s-4,mochitest-e10s-5,web-platform-tests-1,web-platform-tests-2,web-platform-tests-3,web-platform-tests-4,web-platform-tests-5,web-platform-tests-6,web-platform-tests-7,web-platform-tests-8,web-platform-tests-e10s-1,web-platform-tests-e10s-2,web-platform-tests-e10s-3,web-platform-tests-e10s-4,web-platform-tests-e10s-5,web-platform-tests-e10s-6,web-platform-tests-e10s-7,web-platform-tests-e10s-8 -t none

Comment 28

[Josh Matthews [:jdm]]

For the record, I used http://trychooser.pub.build.mozilla.org/ to create that syntax.

Comment 29

[Saurabh Singhal [:saurabhs]]

Thanks Josh. I scheduled the tests again. But, it looks like this time my changes were not pushed. The command says this:

remote: added 1 changesets with 0 changes to 0 files (+1 heads)

How can I push my changes?

Comment 30

[Josh Matthews [:jdm]]

Was there a link to treeherder as part of the output? I am not surprised by the "1 changesets with 0 changes", since no code changed since your previous push - only one extra commit with the trychooser syntax as the commit message.

Comment 31

[Josh Matthews [:jdm]]

Looks like https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb is the correct one.

Comment 32

[Saurabh Singhal [:saurabhs]]

Oh ok. Yes that is the one. I will look at the results.

Comment 33

[Saurabh Singhal [:saurabhs]]

Hi Josh. I was debugging this failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb&selectedJob=27719108&filter-searchStr=e7d624b19ab4d63b029b64af1514107b91eee896

I am unable to understand what the test is doing. Can you give me some pointers?

Comment 34

[Josh Matthews [:jdm]]

Oh, wow. I read your patch over and over, and read the test over and over, and I had to step through the code in a debugger to figure out what was happening. test_fetch_event.html tests various interactions with a service worker which intercepts network requests and provides responses. That means that the XHR to "empty-header" is intercepted in fetch_event_worker.js, which then calls Headers.get looking for the header named "emptyheader" that was originally set on the XHR. Previously, this would return the empty string because Headers.get used to return the first value that matched the requested header name. Now, we return null because Headers.get checks whether the final combined header value is empty. We need to modify the check in Headers.get to only return null if !firstValueFound. Does that make sense?

Comment 35

[Saurabh Singhal [:saurabhs]]

Thanks Josh for debugging this. Your explanation makes sense.
 How did you attach a debugger to the test? This is not working for me: sudo gdb ./mach mochitest dom/workers/test/serviceworkers/test_fetch_event.html

Comment 36

[Saurabh Singhal [:saurabhs]]

Next I was looking at this failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb&selectedJob=27717335&filter-searchStr=Linux%20debug%20W3C%20Web%20Platform%20Tests%20e10s%20W3C%20Web%20Platform%20Tests%20e10s%20W-e10s(4)

When I run the test on my system, it skips the test instead of running it. Is it because the application cache feature is deprecated?

Comment 37

[Josh Matthews [:jdm]]

You can use `./mach mochitest dom/workers/test/serviceworkers/test_fetch_event.html --debugger=gdb` to attach to the parent process, or `MOZ_DEBUG_CHILD_PROCESS=1 ./mach mochitest dom/workers/test/serviceworkers/test_fetch_event.html` and then attach gdb manually to the process id that is printed on stdout if you want to attach to the child process. On the other hand, I used `--disble-e10s --debugger=gdb` so that there was only one process that needed debugging.

Comment 38

[Josh Matthews [:jdm]]

According to https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/html/infrastructure/urls/resolving-urls/query-encoding/utf-16be.html.ini that test is skipped on OS X. You could remove that line in order to run it locally, but given that bug 1295445 exists and the test doesn't use the Headers API, I believe you can ignore that test failure.

Comment 39

[Saurabh Singhal [:saurabhs]]

This job reports memleak : https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb&selectedJob=27717055&filter-searchStr=OS%20X%2010.10%20debug%20Mochitest%20Mochitest%20M(2)

But I am unable to find out which test is run here.

Comment 40

[Saurabh Singhal [:saurabhs]]

I am facing a weird issue. After I submitted the patch to bugzilla, I had to move to a different laptop where I did hg clone and all the stuff again. Also, I took this patch and applied it in the latest mozilla-central repository but testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini file was not there already so the part of the patch where the file was deleted did not apply properly. This caused a test failure here : https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb&selectedJob=27721630&filter-searchStr=Windows%207%20VM%20opt%20W3C%20Web%20Platform%20Tests%20W3C%20Web%20Platform%20Tests%20W(2)
Notice the test unexpected pass. How do I resolve this issue?

Comment 41

[Josh Matthews [:jdm]]

(In reply to Saurabh Singhal [:saurabh] from comment #39)
> This job reports memleak :
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb&selectedJob=2
> 7717055&filter-searchStr=OS%20X%2010.
> 10%20debug%20Mochitest%20Mochitest%20M(2)
> 
> But I am unable to find out which test is run here.

If we aren't seeing similar memory leaks on other platforms for the same testsuite, we can ignore this (otherwise we would need to investigate more). The leak warnings mean "we ran a bunch of tests in a browser window, and when we closed the browser there was memory that was leaked", so it's difficult to associate with a particular test. However, bug 1293324 looks like it matches that leak, and it's a known intermittent leak, so we don't need to worry about it here.

Comment 42

[Josh Matthews [:jdm]]

(In reply to Saurabh Singhal [:saurabh] from comment #40)
> I am facing a weird issue. After I submitted the patch to bugzilla, I had to
> move to a different laptop where I did hg clone and all the stuff again.
> Also, I took this patch and applied it in the latest mozilla-central
> repository but
> testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini file
> was not there already so the part of the patch where the file was deleted
> did not apply properly. This caused a test failure here :
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb&selectedJob=2
> 7721630&filter-
> searchStr=Windows%207%20VM%20opt%20W3C%20Web%20Platform%20Tests%20W3C%20Web%2
> 0Platform%20Tests%20W(2)
> Notice the test unexpected pass. How do I resolve this issue?

I agree that this is odd, because headers-combine.html.ini is definitely still present in mozilla-central: https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini . Make sure that you have an updated mozilla-central and reapply the patch?

Comment 43

[Saurabh Singhal [:saurabhs]]

(In reply to Josh Matthews [:jdm] from comment #42)
> (In reply to Saurabh Singhal [:saurabh] from comment #40)
> > I am facing a weird issue. After I submitted the patch to bugzilla, I had to
> > move to a different laptop where I did hg clone and all the stuff again.
> > Also, I took this patch and applied it in the latest mozilla-central
> > repository but
> > testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini file
> > was not there already so the part of the patch where the file was deleted
> > did not apply properly. This caused a test failure here :
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=9c8e25f06f93e02aac5bdd503105043e30edeceb&selectedJob=2
> > 7721630&filter-
> > searchStr=Windows%207%20VM%20opt%20W3C%20Web%20Platform%20Tests%20W3C%20Web%2
> > 0Platform%20Tests%20W(2)
> > Notice the test unexpected pass. How do I resolve this issue?
> 
> I agree that this is odd, because headers-combine.html.ini is definitely
> still present in mozilla-central:
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/
> fetch/api/headers/headers-combine.html.ini . Make sure that you have an
> updated mozilla-central and reapply the patch?

I cloned the repo in a different directory and made sure that the ini file was there. Then, I applied the patch and got this:

MacBook-Pro:mozilla-central saurabhs$ hg import ~/HeadersgetAll.patch
applying /Users/saurabhs/HeadersgetAll.patch
patching file testing/web-platform/tests/fetch/api/headers/headers-basic.html
Hunk #1 FAILED at 60
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/tests/fetch/api/headers/headers-basic.html.rej
abort: patch failed to apply

On doing hg diff after this, I did not see the headers-combine.html.ini deleted part there although, the file is no longer there in my local repo. It looks like the file got deleted from the disk but did not show up as deleted in the patch. Do you know what can I do here?

Comment 44

[Josh Matthews [:jdm]]

What does `hg status` show?

Comment 45

[Josh Matthews [:jdm]]

Another thing you can try is `hg rm -A testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini` which should properly record the removal of headers-combine.html.ini.

Comment 46

[Saurabh Singhal [:saurabhs]]

(In reply to Josh Matthews [:jdm] from comment #44)
> What does `hg status` show?

MacBook-Pro:headers saurabhs$ hg status
M dom/cache/AutoUtils.cpp
M dom/cache/DBSchema.cpp
M dom/fetch/FetchDriver.cpp
M dom/fetch/Headers.h
M dom/fetch/InternalHeaders.cpp
M dom/fetch/InternalHeaders.h
M dom/flyweb/FlyWebPublishedServer.cpp
M dom/flyweb/HttpServer.cpp
M dom/tests/mochitest/fetch/test_headers_common.js
M dom/tests/mochitest/fetch/test_headers_mainthread.html
M dom/webidl/Headers.webidl
M testing/web-platform/meta/fetch/api/headers/headers-normalize.html.ini
M testing/web-platform/tests/fetch/api/headers/headers-combine.html
M testing/web-platform/tests/fetch/api/headers/headers-normalize.html
! testing/web-platform/meta/fetch/api/headers/headers-combine.html.ini
? testing/web-platform/tests/fetch/api/headers/headers-basic.html.rej

Comment 47

[Josh Matthews [:jdm]]

The `hg status --help` output says "! = missing (deleted by non-hg command, but still tracked)", so I recommend running the `hg rm` command I suggested.

Comment 48

[Saurabh Singhal [:saurabhs]]

Thanks Josh. Your suggestion worked. I have pushed the code to try again.

Comment 49

[Saurabh Singhal [:saurabhs]]

The results are here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3f2108c8fc3&selectedJob=28907216

Comment 50

[Josh Matthews [:jdm]]

Looks good to me!

Comment 51

[Ryan VanderMeulen [:RyanVM]]

The webidl changes from this patch need review from a DOM peer before this can land.

Comment 52

[Josh Matthews [:jdm]]

Comment on attachment 8786799 [details] [diff] [review]
HeadersgetAll.patch

This needs a DOM peer's review of the Header.webidl changes.

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8786799 [details] [diff] [review]
HeadersgetAll.patch

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

I think we should clarify what Cache API Vary header matching should use before landing this.  I filed this spec issue:

https://github.com/w3c/ServiceWorker/issues/991

Overall, though, it looks good.  Sorry for the spec issue.

::: dom/cache/AutoUtils.cpp
@@ +262,5 @@
>                     "TypeUtils::ToPCacheResponseWithoutBody()");
>  
>          ErrorResult headerRv;
>          nsAutoCString value;
> +        requestHeaders->GetFirst(header, value, headerRv);

See my comments for DBSchema.cpp.  This should be `Get()` per the current spec:

https://w3c.github.io/ServiceWorker/#query-cache-algorithm

@@ +269,5 @@
>            MOZ_ASSERT(value.IsEmpty());
>          }
>  
>          nsAutoCString cachedValue;
> +        cachedRequestHeaders->GetFirst(header, cachedValue, headerRv);

Keep `Get()` here.

::: dom/cache/DBSchema.cpp
@@ +1299,5 @@
>                   "TypeUtils::ToPCacheResponseWithoutBody()");
>  
>        ErrorResult errorResult;
>        nsAutoCString queryValue;
> +      queryHeaders->GetFirst(header, queryValue, errorResult);

This seems wrong to me or there is a spec bug.  The service workers spec says to use Headers `get()` which includes combining.  I think we should keep the `Get()` call here instead of using `GetFirst()`.

See:

https://w3c.github.io/ServiceWorker/#query-cache-algorithm

@@ +1306,5 @@
>          MOZ_ASSERT(queryValue.IsEmpty());
>        }
>  
>        nsAutoCString cachedValue;
> +      cachedHeaders->GetFirst(header, cachedValue, errorResult);

Keep `Get()` here.

::: dom/fetch/Headers.h
@@ +85,5 @@
>    {
>      mInternalHeaders->Get(aName, aValue, aRv);
>    }
>  
> +  void GetFirst(const nsACString& aName, nsCString& aValue, ErrorResult& aRv) const

I think this should be `nsACString& aValue`.

::: dom/fetch/InternalHeaders.h
@@ +74,5 @@
>    void Append(const nsACString& aName, const nsACString& aValue,
>                ErrorResult& aRv);
>    void Delete(const nsACString& aName, ErrorResult& aRv);
>    void Get(const nsACString& aName, nsCString& aValue, ErrorResult& aRv) const;
> +  void GetFirst(const nsACString& aName, nsCString& aValue, ErrorResult& aRv) const;

I think this should be `nsACString& aValue).

::: dom/tests/mochitest/fetch/test_headers_mainthread.html
@@ +142,5 @@
>    }, TypeError, "Should not be able to delete immutable headers");
>  
>    checkHas(headers, "foo", "Should be able to check immutable headers");
>    ok(headers.get("foo"), "Should be able to get immutable headers");
> +  ok(headers.get("foo").length, "Should be able to get all immutable headers");

This test assertion seems wrong to me now.  In the old case it was checking the .length of an array.  This is checking the .length of the value returned which is effectively checked by the truthy assertion before it.

Since this is a single value in the 'foo' header, perhaps we should just remove this ok() assertion?

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Initial feedback in the github issue is that we should probably be using the combined value in the Vary header logic.  So my comments about switching from `GetFirst()` back to `Get()` stand.  Sorry for the runaround and thanks for working on this!

Comment 55

[Saurabh Singhal [:saurabhs]]

(In reply to Ben Kelly [:bkelly] from comment #53) 
> ::: dom/fetch/Headers.h
> @@ +85,5 @@
> >    {
> >      mInternalHeaders->Get(aName, aValue, aRv);
> >    }
> >  
> > +  void GetFirst(const nsACString& aName, nsCString& aValue, ErrorResult& aRv) const
> 
> I think this should be `nsACString& aValue`.

Why?

Comment 56

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Saurabh Singhal [:saurabh] from comment #55)
> > > +  void GetFirst(const nsACString& aName, nsCString& aValue, ErrorResult& aRv) const
> > 
> > I think this should be `nsACString& aValue`.
> 
> Why?

Its preferable to use the abstract `nsA*String` string types in method signatures so the caller can decide which concrete string type to actually use.  I think the only time a method should take explicitly `nsString` or `nsCString` is if you require a flat string (not a dependent substring, etc).

I see that the previous `Get()` method also has an `nsCString`.  That could also be switched to `nsACString` I think.

The `GetAll()` method returns an `nsTArray<nsCString>` because it requires a concrete type.

As a side note, why are we leaving `GetAll()` in the code?  Can't that be removed now?

Thanks.

Comment 57

[Saurabh Singhal [:saurabhs]]

(In reply to Ben Kelly [:bkelly] from comment #56)
> As a side note, why are we leaving `GetAll()` in the code?  Can't that be
> removed now?

According to https://bugzilla.mozilla.org/show_bug.cgi?id=1278275#c15 GetAll() is still required.

Comment 58

[Saurabh Singhal [:saurabhs]]

Attachment: HeadersgetAll.patch

Addressing comments from Ben. Thanks for the review.

Comment 59

[Saurabh Singhal [:saurabhs]]

Attachment: HeadersgetAll.patch

Uploading the correct diff.

Comment 60

[Saurabh Singhal [:saurabhs]]

(In reply to Saurabh Singhal [:saurabh] from comment #57)
> (In reply to Ben Kelly [:bkelly] from comment #56)
> > As a side note, why are we leaving `GetAll()` in the code?  Can't that be
> > removed now?
> 
> According to https://bugzilla.mozilla.org/show_bug.cgi?id=1278275#c15
> GetAll() is still required.

Removed GetAll from dom/fetch/Headers.h since the GetAll() being called in AutoUtils.cpp is the one provided by dom/fetch/InternalHeaders.h

Comment 61

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Saurabh Singhal [:saurabh] from comment #60)
> Removed GetAll from dom/fetch/Headers.h since the GetAll() being called in
> AutoUtils.cpp is the one provided by dom/fetch/InternalHeaders.h

You should remove the other one as well.  Just use Get() in AutoUtils and parse the combined value.

Comment 62

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8800754 [details] [diff] [review]
HeadersgetAll.patch

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

Sorry, can you implement comment 61?  Now that Get() combines values we don't need that last GetAll() in AutoUtils.  You can just call Get() and let the parsing code that is already there extract the values.

Comment 63

[Saurabh Singhal [:saurabhs]]

I just found that FetchBody<Derived>::SetMimeType() function in dom/fetch/Fetch.cpp also uses GetAll(). Should I remove this too and add the parsing code here also?

Comment 64

[Ben Kelly [:bkelly, not reviewing]]

I believe you can replace that with Get().  Then for this section:

  // HTTP ABNF states Content-Type may have only one value.
  // This is from the "parse a header value" of the fetch spec.
  if (contentTypeValues.Length() == 1) {
    mMimeType = contentTypeValues[0];
    ToLowerCase(mMimeType);
  }

You can instead search for "," to see if there is more than one value. Sound reasonable?

Comment 65

[Saurabh Singhal [:saurabhs]]

Attachment: HeadersgetAll.patch

I could not figure out a good place to put the parsing code. So, I have duplicated it for now in AutoUtils.cpp and TypeUtils.cpp

Comment 66

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8801564 [details] [diff] [review]
HeadersgetAll.patch

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

Sorry, I think there was a miscommunication.  The parsing code is already in the code.  See:

https://dxr.mozilla.org/mozilla-central/source/dom/cache/AutoUtils.cpp#255
https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.cpp#56

What it was doing before was getting all headers, but then parsing each one.  That was because we couldn't tell if a single value returned from Headers.getAll() was combined or not.

So you don't need to implement your own parsing here.  Just remove the outer loop from the strtok() loops that are already there.

Comment 67

[Saurabh Singhal [:saurabhs]]

Attachment: HeadersgetAll.patch

Fix the parsing code in AutoUtils.cpp and TypeUtils.cpp

Comment 68

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8801810 [details] [diff] [review]
HeadersgetAll.patch

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

Thanks!

In the future we may want to rejigger the logic so Headers::Append creates the combined string immediately instead of recalculating it on every Get().  That can be done in a follow-up, though.

Comment 69

[Saurabh Singhal [:saurabhs]]

Thanks Ben! I have pushed the changes to try. The results are here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b7a8502c1d1271d1114cd49bc6264d3b7e4f39d

Comment 70

[Ben Kelly [:bkelly, not reviewing]]

Looks green.  Thanks again!

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/add68bb69193
Update Headers::GetAll and Headers::Get to match the specification. r=jdm, r=bkelly

Comment 72

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/add68bb69193

Comment 73

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/headers-getall-has-been-removed-in-favour-of-get-now-returning-all-values/

Comment 74

[Chris Mills [:cmills]]

I have documented this change, updating the relevant reference/tutorial pages:
https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API
https://developer.mozilla.org/en-US/docs/Web/API/Headers
https://developer.mozilla.org/en-US/docs/Web/API/Headers/get
https://developer.mozilla.org/en-US/docs/Web/API/Headers/getAll
https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch

I have also added a note to the Fx52 release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/52#Service_Workers_and_Fetch

Let me know if these look OK. Thanks!