Closed Bug 1029620 Opened 5 years ago Closed 5 years ago

Implement Headers interface from Fetch specification

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: nsm, Assigned: bkelly)

References

Details

(Keywords: dev-doc-complete)

Attachments

(7 files, 29 obsolete files)

7.27 KB, patch
Ehsan
: review+
nsm
: checkin+
Details | Diff | Splinter Review
1.80 KB, patch
Ehsan
: review+
nsm
: checkin+
Details | Diff | Splinter Review
3.27 KB, patch
Ehsan
: review+
bkelly
: checkin+
Details | Diff | Splinter Review
6.07 KB, patch
bkelly
: review+
bkelly
: checkin+
Details | Diff | Splinter Review
18.46 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
14.64 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
12.00 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
Splitting up since it's self contained
Summary: Implement Headers class from Fetch specification → Implement Headers interface from Fetch specification
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Assignee: bkelly → nsm.nikhil
Assignee: nsm.nikhil → bkelly
Work-in-progress patch that implements the majority of the Headers functionality.  Still todo:

1) Add some additional validation for name/value format.
2) Implement the construction fill algorithm.
3) Add tests.
Attachment #8445293 - Attachment is obsolete: true
Note that the forbidden header list here is the same as we have for XMLHttpRequest / CORS. Ideally we'd share that data somehow.
Depends on: 1029812
This implementation should be complete.  Just need to add some tests and then will ask for review.
Attachment #8445340 - Attachment is obsolete: true
Cleanup and expose guard attribute in preparation for writing tests.
Attachment #8446173 - Attachment is obsolete: true
Fixes based on testing and try build failures.  More cleanup.
Attachment #8446271 - Attachment is obsolete: true
Tests and new try build:

  https://tbpl.mozilla.org/?tree=Try&rev=ab6aa1500642

Will flag for review if this is green.
Some updates to handle include oddities on other platforms in the try build.

1) I need to explicitly include ErrorResult.h now that I moved the files into dom/fetch and I am getting fewer includes for free through unified builds.
2) Also, I think rejiggering the unified builds causes nsPluginArray.h to not get nsMimeTypeArray.h for free.  So I had to add that include there instead of the forward declare it thought it was getting away with.

Not sure if (2) should be done as a separate patch/bug, but just putting it in this patch for now.

New try:

  https://tbpl.mozilla.org/?tree=Try&rev=1b01c2c2811c
Attachment #8446891 - Attachment is obsolete: true
Hmm, lets put those changes in the right patch this time.  Also, another attempt to fix broken windows non-unified builds.

https://tbpl.mozilla.org/?tree=Try&rev=0b5ee6d70857
Attachment #8446959 - Attachment is obsolete: true
Having a lot of trouble with my patch queue tonight.  Think this one is right:

https://tbpl.mozilla.org/?tree=Try&rev=f761c1880c0e
Attachment #8446977 - Attachment is obsolete: true
Only failing to build on windows xp.  I think its a namespace issue.  Lets see if this fixes it.

https://tbpl.mozilla.org/?tree=Try&rev=ae83fe7cb9d8
Attachment #8446978 - Attachment is obsolete: true
Attachment #8446172 - Flags: review?(ehsan)
Attachment #8446892 - Flags: review?(ehsan)
Attachment #8447168 - Flags: review?(ehsan)
Attachment #8447168 - Flags: superreview?(jst)
Comment on attachment 8446892 [details] [diff] [review]
P3 Test new fetch Headers DOM object. (v0)

The update to test_interfaces.html is in this patch, so sr? here as well.
Attachment #8446892 - Flags: superreview?(jst)
Note, there is an open issue to add iterator support to Headers.  See:

  https://www.w3.org/Bugs/Public/show_bug.cgi?id=26102

The blink IDL added a forEach method for this here:

  http://src.chromium.org/viewvc/blink/trunk/Source/modules/serviceworkers/Headers.idl

Discussing on IRC with Boris and Anne, however, its unclear exactly how the iterator will behave.  And it will not be a forEach() method in gecko, but an @@iterator.

I think we decided to wait on this for now until its more clear how the iterator behaves and it actually lands in the spec.
Comment on attachment 8446892 [details] [diff] [review]
P3 Test new fetch Headers DOM object. (v0)

I uncovered some corner cases talking with Anne this morning on IRC.  Dropping flags until I can address those.  Sorry for the bug spam.
Attachment #8446892 - Flags: superreview?(jst)
Attachment #8446892 - Flags: review?(ehsan)
Attachment #8447168 - Flags: superreview?(jst)
Attachment #8447168 - Flags: review?(ehsan)
Things I need to fix:

1) Use nsStringCaseInsensitiveHashKey so that header names with different cases collapse into the same list of values.

2) Modify IsSimpleHeader() to parse the value for ContentType similar to:

     http://lxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#2869
Comment on attachment 8446172 [details] [diff] [review]
P1 Refactor XHR to move forbidden header checks to nsContentUtils.

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

::: content/base/public/nsContentUtils.h
@@ +2163,5 @@
> +   */
> +  static bool IsForbiddenSystemRequestHeader(const nsACString& aHeader);
> +
> +  /**
> +   * Returns whether a given header is forbidden for an XHR or fetch

Nit: s/header/HTTP header/ in all of these comments please.
Attachment #8446172 - Flags: review?(ehsan) → review+
Ehsan, Fetch calls them headers because these are used outside the context of HTTP too.
(In reply to comment #18)
> Ehsan, Fetch calls them headers because these are used outside the context of
> HTTP too.

Like where?  Regardless, that was just a nit, please ignore it if it doesn't make sense. :-)
E.g. you can fetch a data URL and the response will have headers (such as Content-Type). Only a subset of fetches involves HTTP.
Some more preparation patches I split out from the main patch.  These are reviewable while I fix up the last couple issues in the main patch.
Attachment #8447429 - Flags: review?(ehsan)
Attachment #8447168 - Attachment is obsolete: true
Attachment #8446892 - Attachment is obsolete: true
I'll try to get to this on Monday.
Comment on attachment 8447429 [details] [diff] [review]
P2 Include nsIChannel.h as nsCOMPtr<> requires full type definition. (v0)

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

It's slightly better to avoid adding the include here and make ~nsCORSListenerProxy out of line (that is the function that needs to call Release on the nsCOMPtrs.)
Attachment #8447429 - Flags: review?(ehsan) → review-
Comment on attachment 8447430 [details] [diff] [review]
P3 Include nsMimeTypeArray.h as nsRefPtr<> requires full type definition.

This is actually obsoleted by bug 1032417.  :-)
Attachment #8447430 - Attachment is obsolete: true
Attachment #8447430 - Flags: review?(ehsan)
Attachment #8447431 - Flags: review?(ehsan) → review+
Modified to out-of-line the destructor instead of adding nsIChannel.h to the header.
Attachment #8447429 - Attachment is obsolete: true
Attachment #8452602 - Flags: review?(ehsan)
Attachment #8452602 - Flags: review?(ehsan) → review+
Went ahead and landed first two patches as they are very simple and standalone:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/de0cdabaf412
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c304aa9a55e4

Leave-open until remaining patches are reviewed and land.
Keywords: leave-open
Renumber patch.  Carry r+ forward.
Attachment #8447431 - Attachment is obsolete: true
Attachment #8452645 - Flags: review+
Renumber patch.  Implement header name case insensitivity by lower casing in each method.

I still need to fix IsSimpleHeader() to parse the value.
Attachment #8447433 - Attachment is obsolete: true
Expand tests to exercise name case insensitivity.  Verify guard attribute is not visible to content.
Attachment #8447434 - Attachment is obsolete: true
Parsing for IsSimpleHeader() was also done in XHR, so decided to factor that out as well as the forbidden header checking.
Attachment #8453251 - Flags: review?(ehsan)
This patch adds parsing the content-type value so that we can ignore the charset parameter when checking for simple non-CORS headers.

I believe this is now ready for review.
Attachment #8452649 - Attachment is obsolete: true
Attachment #8453255 - Flags: superreview?(jonas)
Attachment #8453255 - Flags: review?(ehsan)
Verify charset parameters can be used with content-type values on non-CORS request headers.
Attachment #8452651 - Attachment is obsolete: true
Attachment #8453257 - Flags: review?(ehsan)
Attachment #8453251 - Flags: review?(ehsan) → review+
Comment on attachment 8453255 [details] [diff] [review]
P5 Implement Fetch Headers DOM object. (v10)

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

r=me with the comments below addressed/explained as appropriate.  Thanks!

::: dom/bindings/Errors.msg
@@ +56,5 @@
>  MSG_DEF(MSG_INVALID_READ_SIZE, 0, "0 (Zero) is not a valid read size.")
> +MSG_DEF(MSG_HEADERS_IMMUTABLE, 0, "Headers are immutable and cannot be modified.")
> +MSG_DEF(MSG_INVALID_HEADER_NAME, 1, "{0} is an invalid header name.")
> +MSG_DEF(MSG_INVALID_HEADER_VALUE, 1, "{0} is an invalid header value.")
> +MSG_DEF(MSG_INVALID_HEADER_SEQUENCE, 0, "Headers require name/value tuples when being initialized by a sequence.")

I don't know whether we're supposed to generate custom type errors like this..  Please check with bz.

::: dom/fetch/Headers.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "Headers.h"

Nit: mozilla/dom/Headers.h.

@@ +19,5 @@
> +namespace dom {
> +
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(Headers)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(Headers)
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(Headers)

You need to traverse the owner once you made it a strong ref.

@@ +57,5 @@
> +void
> +Headers::Append(const nsACString& aName, const nsACString& aValue,
> +                ErrorResult& aRv)
> +{
> +  nsCString lowerName;

nsAutoCString here and everywhere else below when you create strings on the stack please.

@@ +99,5 @@
> +  }
> +
> +  ValueList* list = mMap.Get(lowerName);
> +  if (!list || list->IsEmpty()) {
> +    aValue.SetIsVoid(true);

Does this map properly to returning null?  I didn't double check...

@@ +134,5 @@
> +  if (IsInvalidName(lowerName, aRv)) {
> +    return false;
> +  }
> +
> +  return mMap.Get(lowerName);

Nit: !!

@@ +190,5 @@
> +  return aName.EqualsLiteral("accept") ||
> +         aName.EqualsLiteral("accept-language") ||
> +         aName.EqualsLiteral("content-language") ||
> +         (aName.EqualsLiteral("content-type") &&
> +          (!aValue || nsContentUtils::IsAllowedNonCorsContentType(*aValue)));

This will allow Content-Type headers with an empty value, right?  If yes, we should fix that.

@@ +197,5 @@
> +//static
> +bool
> +Headers::IsInvalidName(const nsACString& aName, ErrorResult& aRv)
> +{
> +  if (!IsValidHTTPToken(aName)) {

This should live in nsContentUtils...  File a follow-up please?

@@ +210,5 @@
> +// static
> +bool
> +Headers::IsInvalidValue(const nsACString& aValue, ErrorResult& aRv)
> +{
> +  if (aValue.FindChar('\n') != -1 || aValue.FindChar('\r') != -1) {

Hmm, is this enough?  I don't think this matches http://tools.ietf.org/html/rfc2616#section-4.2 very closely.

@@ +253,5 @@
> +    if (tuple.Length() != 2) {
> +      aRv.ThrowTypeError(MSG_INVALID_HEADER_SEQUENCE);
> +      return;
> +    }
> +    Set(tuple[0], tuple[1], aRv);

You should Append() here, not Set().  Also, please add a test which would have caught this.

@@ +258,5 @@
> +  }
> +}
> +
> +void
> +Headers::Fill(const MozMap<nsCString>& aInit, ErrorResult& aRv)

Does MozMap deal correctly with byte strings?  If not, please file a follow-up on that.

@@ +263,5 @@
> +{
> +  nsTArray<nsString> keys;
> +  aInit.GetKeys(keys);
> +  for (uint32_t i = 0; i < keys.Length() && !aRv.Failed(); ++i) {
> +    Set(NS_ConvertUTF16toUTF8(keys[i]), aInit.Get(keys[i]), aRv);

Again, you want Append() here.  And please add a test for this too!

::: dom/fetch/Headers.h
@@ +8,5 @@
> +#define mozilla_dom_Headers_h
> +
> +#include "mozilla/dom/HeadersBinding.h"
> +#include "nsClassHashtable.h"
> +#include "nsContentUtils.h"

It would be nice to not include this giant header in Headers.h...

@@ +29,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(Headers)
> +
> +private:
> +  nsISupports* mOwner;

This should be a strong ref.

@@ +35,5 @@
> +  typedef nsTArray<nsCString> ValueList;
> +  nsClassHashtable<nsCStringHashKey, ValueList> mMap;
> +
> +public:
> +  Headers(nsISupports* aOwner, HeadersGuardEnum aGuard=HeadersGuardEnum::None)

Nit: explicit, and spaces around =.

::: dom/fetch/moz.build
@@ +13,5 @@
> +]
> +
> +FAIL_ON_WARNINGS = True
> +MSVC_ENABLE_PGO = True
> +FINAL_LIBRARY = 'gklayout'

Why does this not go in libxul?  Please ask for a build system peer to review this.  I'm not sure what the right thing to do here is.
Attachment #8453255 - Flags: review?(ehsan) → review+
Comment on attachment 8453257 [details] [diff] [review]
P6 Test new fetch Headers DOM object. (v3)

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

r=me with the below fixed.  Please feel free to ask for review again if testing the worker path changes things too much.

::: dom/tests/mochitest/fetch/test_headers.html
@@ +8,5 @@
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +</head>
> +<body>
> +<script class="testbody" type="text/javascript">
> +SimpleTest.waitForExplicitFinish();

In addition to the coverage requested below, we should test the behavior of this code inside workers too.  You should be able to refactor most of the below in a .js file and then test both the main thread path and the worker path.

::: dom/workers/test/fetch/mochitest.ini
@@ +1,5 @@
> +[DEFAULT]
> +support-files =
> +  worker_interfaces.js
> +
> +[test_interfaces.html]

Please call this test_fetchinterfaces.html.
Attachment #8453257 - Flags: review?(ehsan) → review+
Attachment #8459901 - Attachment description: Bug 1029620 P4 Move IsValidHTTPToken() to nsContentUtils (v0) → P4 Move IsValidHTTPToken() to nsContentUtils (v0)
This implements most of the review feedback.  It also converts from using a hash table to a simple list in order to maintain absolute ordering as defined by the spec.

The main thing left to do is flesh out the validation of value strings.  I'm hopeful I can reuse some code from elsewhere in the tree, although I did not find it before.
Attachment #8453255 - Attachment is obsolete: true
Attachment #8453255 - Flags: superreview?(jonas)
Test case the case where a sequence of tuples is used to initialize a Headers object and a header name is duplicated.

I still need to refactor this a bit in order for the tests to run in both main thread and worker contexts.
Attachment #8453257 - Attachment is obsolete: true
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #40)
> ::: dom/bindings/Errors.msg
> @@ +56,5 @@
> >  MSG_DEF(MSG_INVALID_READ_SIZE, 0, "0 (Zero) is not a valid read size.")
> > +MSG_DEF(MSG_HEADERS_IMMUTABLE, 0, "Headers are immutable and cannot be modified.")
> > +MSG_DEF(MSG_INVALID_HEADER_NAME, 1, "{0} is an invalid header name.")
> > +MSG_DEF(MSG_INVALID_HEADER_VALUE, 1, "{0} is an invalid header value.")
> > +MSG_DEF(MSG_INVALID_HEADER_SEQUENCE, 0, "Headers require name/value tuples when being initialized by a sequence.")
> 
> I don't know whether we're supposed to generate custom type errors like
> this..  Please check with bz.

Boris, can you comment on the proper way to handle Errors.msg?  If I do update it, do I need to do anything for l10n?
Flags: needinfo?(bzbarsky)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #40)
> @@ +210,5 @@
> > +// static
> > +bool
> > +Headers::IsInvalidValue(const nsACString& aValue, ErrorResult& aRv)
> > +{
> > +  if (aValue.FindChar('\n') != -1 || aValue.FindChar('\r') != -1) {
> 
> Hmm, is this enough?  I don't think this matches
> http://tools.ietf.org/html/rfc2616#section-4.2 very closely.

Actually, I am pretty close to what necko currently does.  See:

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

I can add a check for a null byte, but I think parsing for quotes strings might be a bit heavyweight here.

Ehsan, what do you think?
Flags: needinfo?(ehsan)
Comment on attachment 8459901 [details] [diff] [review]
P4 Move IsValidHTTPToken() to nsContentUtils (v0)

Drop review flag.  I just noticed that there is a duplicate implementation in necko.  Lets see if we can unify this a bit.
Attachment #8459901 - Flags: review?(ehsan)
The Errors.msg parts are fine and do not need (or indeed want) localizing.  Why do you need all that headerFile/nativeType stuff in Bindings.conf, though?  And why do you need a separate worker descriptor for that matter?
Flags: needinfo?(bzbarsky)
(In reply to Ben Kelly [:bkelly] from comment #47)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #40)
> > @@ +210,5 @@
> > > +// static
> > > +bool
> > > +Headers::IsInvalidValue(const nsACString& aValue, ErrorResult& aRv)
> > > +{
> > > +  if (aValue.FindChar('\n') != -1 || aValue.FindChar('\r') != -1) {
> > 
> > Hmm, is this enough?  I don't think this matches
> > http://tools.ietf.org/html/rfc2616#section-4.2 very closely.
> 
> Actually, I am pretty close to what necko currently does.  See:
> 
>  
> http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> HttpBaseChannel.cpp#1126
> 
> I can add a check for a null byte, but I think parsing for quotes strings
> might be a bit heavyweight here.
> 
> Ehsan, what do you think?

OK, sounds fine either way if Necko doesn't handle it either.  Worth filing a follow-up on that though.
Flags: needinfo?(ehsan)
This patch refactors existing code that performs HTTP token and header value validation.  Now these routines are solely implement in nsHttp and exposed outside of necko via nsNetUtil.h.
Attachment #8459901 - Attachment is obsolete: true
Attachment #8460655 - Flags: review?(mcmanus)
Attachment #8460655 - Flags: review?(ehsan)
Update to use the nsNetUtil functions and for Boris's comments on Bindings.conf.
Attachment #8459905 - Attachment is obsolete: true
Attachment #8460658 - Flags: review?(ehsan)
Refactor tests to run as many as possible on the worker thread.  Unfortunately all could not be run there since SpecialPowers are required to wrap the Headers object when twiddling the guard attribute.

I also just consolidated this all into dom/tests/mochitest/fetch instead of splitting the tests with dom/workers/test/fetch.
Attachment #8459907 - Attachment is obsolete: true
Attachment #8460661 - Flags: review?(ehsan)
Comment on attachment 8459904 [details] [diff] [review]
P5 Create dom.fetch.enabled pref, defaulted to false. (v3)

This was previously r+'d by ehsan.  Lost the flag when I renumbered earlier.
Attachment #8459904 - Flags: review+
Comment on attachment 8460655 [details] [diff] [review]
P4 Make HTTP token and header value validation accessible via nsNetUtil.h (v1)

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

The DOM parts look good!

::: netwerk/base/public/nsNetUtil.h
@@ +2421,5 @@
>  #endif
>  }
>  
> +bool NS_IsValidHTTPHeaderValue(const nsACString& aValue);
> +bool NS_IsValidHTTPToken(const nsACString& aToken);

Note that all of the other functions in this header are declared inline.  Not sure why that is, though.

::: netwerk/base/src/moz.build
@@ +46,5 @@
>      'nsMediaFragmentURIParser.cpp',
>      'nsMIMEInputStream.cpp',
>      'nsNetAddr.cpp',
>      'nsNetStrings.cpp',
> +    'nsNetUtil.cpp',

nsNetUtil.cpp is missing from the patch.
Attachment #8460655 - Flags: review?(ehsan) → review+
Comment on attachment 8460658 [details] [diff] [review]
P6 Implement Fetch Headers DOM object. (v12)

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

Please submit interdiffs for previously reviewed patches in the future...

::: dom/fetch/Headers.h
@@ +30,5 @@
> +
> +private:
> +  struct Entry
> +  {
> +    explicit

You don't need explicit here.

@@ +48,5 @@
> +  nsTArray<Entry> mList;
> +
> +public:
> +  explicit
> +  Headers(nsISupports* aOwner, HeadersGuardEnum aGuard = HeadersGuardEnum::None)

Nit: please put explicit on the same line.

@@ +77,5 @@
> +  virtual JSObject* WrapObject(JSContext* aCx);
> +  nsISupports* GetParentObject() const { return mOwner; }
> +
> +private:
> +  Headers(const Headers& aOther);

Nit: please use MOZ_DELETE.

::: dom/workers/RegisterBindings.cpp
@@ +80,5 @@
>      return false;
>    }
>  
> +  if (DOMFetchEnabled()) {
> +    if (!HeadersBinding::GetConstructorObject(aCx, aGlobal)) {

Aren't you supposed to use HeadersBinding_workers here?  Please check with someone what the right thing to do here is.

@@ +81,5 @@
>    }
>  
> +  if (DOMFetchEnabled()) {
> +    if (!HeadersBinding::GetConstructorObject(aCx, aGlobal)) {
> +      return nullptr;

return false;
Attachment #8460658 - Flags: review?(ehsan) → review+
Update to include the missing nsNetUtil.cpp.  Also add some comments to the new methods in nsNetUtil.h.

Ehsan, I created a .cpp for nsNetUtil because these methods need to use the necko internal-only class nsHttp.  The guidance I got on IRC was to avoid exposing that to the world through nsNetUtil.h.
Attachment #8460655 - Attachment is obsolete: true
Attachment #8460655 - Flags: review?(mcmanus)
Attachment #8460872 - Flags: review+
Attachment #8460872 - Flags: review?(mcmanus)
Comment on attachment 8460655 [details] [diff] [review]
P4 Make HTTP token and header value validation accessible via nsNetUtil.h (v1)

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

thanks for this.. I'd like one minor change.

::: netwerk/protocol/http/nsHttp.cpp
@@ +244,5 @@
>  }
>  
> +// static
> +bool
> +nsHttp::IsValidHeaderValue(const nsACString &s)

This is actually more than valid.. let's rename it "IsReasonableHeaderValue()" and have it also include IsValidToken as the first part of it.
Attachment #8460655 - Attachment is obsolete: false
Attachment #8460655 - Flags: review+ → review?(ehsan)
Comment on attachment 8460661 [details] [diff] [review]
P7 Test new fetch Headers DOM object. (v5)

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

::: dom/tests/mochitest/fetch/test_headers.html
@@ +40,5 @@
> +    var mainThreadScript = document.createElement("script");
> +    mainThreadScript.setAttribute("src", "test_headers_mainthread.js");
> +    mainThreadScript.onload = function() {
> +      document.head.removeChild(mainThreadScript);
> +      document.head.removeChild(commonScript);

Why do you need to remove these script elements?

::: dom/tests/mochitest/fetch/test_headers_mainthread.js
@@ +137,5 @@
> +
> +TestRequestHeaders();
> +TestRequestNoCorsHeaders();
> +TestResponseHeaders();
> +TestImmutableHeaders();

It would be much simpler to call testOnWorker() directly here, and just include this and test_headers_common.js as static script elements in the test file, and remove all of the main thread script loading logic from the test file!
Attachment #8460661 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #56)
> ::: dom/workers/RegisterBindings.cpp
> @@ +80,5 @@
> >      return false;
> >    }
> >  
> > +  if (DOMFetchEnabled()) {
> > +    if (!HeadersBinding::GetConstructorObject(aCx, aGlobal)) {
> 
> Aren't you supposed to use HeadersBinding_workers here?  Please check with
> someone what the right thing to do here is.

I thought so as well, but Boris pointed out in comment 49 that I didn't need separate worker classes to have the binding in both main thread and worker.  If you look at some of the other entries in RegisterBinding.cpp you can see that, indeed, many do not use _workers.
(In reply to Patrick McManus [:mcmanus] from comment #58)
> ::: netwerk/protocol/http/nsHttp.cpp
> @@ +244,5 @@
> >  }
> >  
> > +// static
> > +bool
> > +nsHttp::IsValidHeaderValue(const nsACString &s)
> 
> This is actually more than valid.. let's rename it
> "IsReasonableHeaderValue()" and have it also include IsValidToken as the
> first part of it.

Hmm, wouldn't this block a lot of current values that are permitted.  For example, IsValidToken prevents separators which are necessary in header values, no?

Also, did I understand the flag update correctly?  You want ehsan to review this again after this change?

Thanks!
Flags: needinfo?(mcmanus)
Attachment #8460655 - Attachment is obsolete: true
Attachment #8460655 - Flags: review?(ehsan)
Attachment #8460655 - Attachment is obsolete: true
Attachment #8460655 - Flags: review?(ehsan)
Flags: needinfo?(mcmanus)
Comment on attachment 8460872 [details] [diff] [review]
P4 Make HTTP token and header value validation accessible via nsNetUtil.h (v2)

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

(I attached this to the wrong version of the patch before - sorry)

thanks for this.. I'd like one minor change.

::: netwerk/protocol/http/nsHttp.cpp
@@ +244,5 @@
>  }
>  
> +// static
> +bool
> +nsHttp::IsValidHeaderValue(const nsACString &s)

This is actually more than valid.. let's rename it "IsReasonableHeaderValue()" and have it also include IsValidToken as the first part of it.
Attachment #8460872 - Flags: review?(mcmanus) → review+
talked about this on irc..(In reply to Ben Kelly [:bkelly] from comment #61)
> (In reply to Patrick McManus [:mcmanus] from comment #58)
> > ::: netwerk/protocol/http/nsHttp.cpp
> > @@ +244,5 @@
> > >  }
> > >  
> > > +// static
> > > +bool
> > > +nsHttp::IsValidHeaderValue(const nsACString &s)
> > 
> > This is actually more than valid.. let's rename it
> > "IsReasonableHeaderValue()" and have it also include IsValidToken as the
> > first part of it.
> 
> Hmm, wouldn't this block a lot of current values that are permitted.  For
> example, IsValidToken prevents separators which are necessary in header
> values, no?

my mistake - confusion over whether name or value was being checked.

>
Rename IsValidHeaderValue to IsReasonableHeaderValue and added comments that "reasonable" means we're not fully parsing to validate quoted strings.
Attachment #8460872 - Attachment is obsolete: true
Attachment #8460969 - Flags: review+
Attachment #8460658 - Attachment is obsolete: true
Attachment #8460971 - Flags: review+
Removed unnecessary code that removes script elements at the end of the test.  I left the dynamic script loading, though, as it needs to run after flipping the fetch pref.  I have been informed we typically use an iframe for this purpose, but it does not seem worth rewriting the test at this point as they both work.
Attachment #8460661 - Attachment is obsolete: true
Attachment #8460973 - Flags: review+
Attachment #8460971 - Flags: superreview?(jonas)
All review feedback addressed.  Just need SR now.  I'd like to do a try run as well, but the tree is closed for an infrastructure problem at the moment.
> I'm not sure what the right thing to do here is.

Because libgklayout and libnecko used to be separate libraries.  Nowadays, I'm not sure what the state is on non-libxul use of nsNetUtil.h.
Speaking of which, creation of nsNetUtil.cpp should probably get necko module owner review....
(In reply to Boris Zbarsky [:bz] from comment #69)
> Speaking of which, creation of nsNetUtil.cpp should probably get necko
> module owner review....

Patrick McManus r+'d it in comment 62.  Or do I need double review?
(In reply to Boris Zbarsky [:bz] from comment #68)
> > I'm not sure what the right thing to do here is.
> 
> Because libgklayout and libnecko used to be separate libraries.  Nowadays,
> I'm not sure what the state is on non-libxul use of nsNetUtil.h.

Seem gklayout is gone as of bug 1041860.  Also, Mike told me just to use xul here.
> Patrick McManus r+'d it in comment 62.

That's totally good enough.  ;)

> Seem gklayout is gone as of bug 1041860.

It's gone as of way earlier than that.
Fix non-unified build bustage revealed by try.  New try:

  https://tbpl.mozilla.org/?tree=Try&rev=3c186f951cc1
Attachment #8460969 - Attachment is obsolete: true
Attachment #8461063 - Flags: review+
Pushed P3 and P4 as they are standalone and I'd like to avoid bit rot while waiting for final review.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3baea94810b1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1934766cc3b3
Attachment #8453251 - Flags: checkin+
Attachment #8459904 - Flags: checkin+
Attachment #8460971 - Flags: superreview?(jonas) → superreview?(jst)
Attachment #8460971 - Flags: superreview?(jst) → superreview+
Headers interface documented here: https://developer.mozilla.org/en-US/docs/Web/API/Headers. Can I get a tech review please? Thanks!
The docs should mention guards, which are not available as an API but affect whether set/delete/append actually mutate the header. https://fetch.spec.whatwg.org/#concept-headers-guard
r+ otherwise.
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #80)
> The docs should mention guards, which are not available as an API but affect
> whether set/delete/append actually mutate the header.
> https://fetch.spec.whatwg.org/#concept-headers-guard
> r+ otherwise.

Thanks. I've updated the page with a description of guard:

https://developer.mozilla.org/en-US/docs/Web/API/Headers

And added a glossary entry with a longer description:

https://developer.mozilla.org/en-US/docs/Glossary/Guard

Of course, rabbit holing being what it is, I've also added these glossary entries ;-)

https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_header_name
https://developer.mozilla.org/en-US/docs/Glossary/Simple_header
https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_response_header_name

Can you check these and let me know if they make sense and say all that needs to be said?
The immutable guard is used on Response.redirect() and Response.error(). Everything else looks good. Thanks!
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #82)
> The immutable guard is used on Response.redirect() and Response.error().
> Everything else looks good. Thanks!

I've added a line to clarify this:

https://developer.mozilla.org/en-US/docs/Glossary/Guard

See 4th bullet down.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.