Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Description • 10 years ago

Splitting up since it's self contained

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 2 • 10 years ago

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.

	Anne (:annevk)
	Comment 3 • 10 years ago

Note that the forbidden header list here is the same as we have for XMLHttpRequest / CORS. Ideally we'd share that data somehow.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 5 • 10 years ago

This implementation should be complete.  Just need to add some tests and then will ask for review.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 6 • 10 years ago

Cleanup and expose guard attribute in preparation for writing tests.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 7 • 10 years ago

Fixes based on testing and try build failures.  More cleanup.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 8 • 10 years ago

Tests and new try build:

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

Will flag for review if this is green.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 9 • 10 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 10 • 10 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 11 • 10 years ago

Having a lot of trouble with my patch queue tonight.  Think this one is right:

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 12 • 10 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 13 • 10 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 14 • 10 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 15 • 10 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 16 • 10 years ago

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

	(no longer active)
	Comment 17 • 10 years ago

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.

	Anne (:annevk)
	Comment 18 • 10 years ago

Ehsan, Fetch calls them headers because these are used outside the context of HTTP too.

	(no longer active)
	Comment 19 • 10 years ago

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

	Anne (:annevk)
	Comment 20 • 10 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 21 • 10 years ago

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.

	(no longer active)
	Comment 26 • 10 years ago

I'll try to get to this on Monday.

	(no longer active)
	Comment 27 • 10 years ago

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

	(no longer active)
	Comment 28 • 10 years ago

Comment on attachment 8447430 [details] [diff] [review]
P3 Include nsMimeTypeArray.h as nsRefPtr<> requires full type definition.

This is actually obsoleted by bug 1032417.  :-)

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 29 • 10 years ago

Modified to out-of-line the destructor instead of adding nsIChannel.h to the header.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 30 • 10 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 31 • 10 years ago

Renumber patch.  Carry r+ forward.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 32 • 10 years ago

Renumber patch.  Implement header name case insensitivity by lower casing in each method.

I still need to fix IsSimpleHeader() to parse the value.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 33 • 10 years ago

Expand tests to exercise name case insensitivity.  Verify guard attribute is not visible to content.

	Carsten Book [:Tomcat]
	Comment 34 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/de0cdabaf412
https://hg.mozilla.org/mozilla-central/rev/c304aa9a55e4

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 35 • 10 years ago

Parsing for IsSimpleHeader() was also done in XHR, so decided to factor that out as well as the forbidden header checking.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 37 • 10 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 38 • 10 years ago

Verify charset parameters can be used with content-type values on non-CORS request headers.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 39 • 10 years ago

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

	(no longer active)
	Comment 40 • 10 years ago

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.

	(no longer active)
	Comment 41 • 10 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 44 • 10 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 45 • 10 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 46 • 10 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 47 • 10 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 48 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 49 • 10 years ago

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?

	(no longer active)
	Comment 50 • 10 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 51 • 10 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 52 • 10 years ago

Update to use the nsNetUtil functions and for Boris's comments on Bindings.conf.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 53 • 10 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 54 • 10 years ago

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.

	(no longer active)
	Comment 55 • 10 years ago

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.

	(no longer active)
	Comment 56 • 10 years ago

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;

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 57 • 10 years ago

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.

	Patrick McManus [:mcmanus]
	Comment 58 • 10 years ago

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.

	(no longer active)
	Comment 59 • 10 years ago

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!

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 60 • 10 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 61 • 10 years ago

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

	Patrick McManus [:mcmanus]
	Comment 62 • 10 years ago

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.

	Patrick McManus [:mcmanus]
	Comment 63 • 10 years ago

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.

>

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 64 • 10 years ago

Rename IsValidHeaderValue to IsReasonableHeaderValue and added comments that "reasonable" means we're not fully parsing to validate quoted strings.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 66 • 10 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 67 • 10 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 68 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 69 • 10 years ago

Speaking of which, creation of nsNetUtil.cpp should probably get necko module owner review....

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 70 • 10 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 71 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 72 • 10 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 73 • 10 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 74 • 10 years ago

Fix non-unified build bustage revealed by try.  New try:

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 75 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 76 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/3baea94810b1
https://hg.mozilla.org/mozilla-central/rev/1934766cc3b3

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 77 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/35147019d5e9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ce5290527b36
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d8dfc26f9295

	Carsten Book [:Tomcat]
	Comment 78 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/35147019d5e9
https://hg.mozilla.org/mozilla-central/rev/ce5290527b36
https://hg.mozilla.org/mozilla-central/rev/d8dfc26f9295

	Chris Mills [:cmills]
	Comment 79 • 9 years ago

Headers interface documented here: https://developer.mozilla.org/en-US/docs/Web/API/Headers. Can I get a tech review please? Thanks!

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 80 • 9 years ago

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.

	Chris Mills [:cmills]
	Comment 81 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 82 • 9 years ago

The immutable guard is used on Response.redirect() and Response.error(). Everything else looks good. Thanks!

	Chris Mills [:cmills]
	Comment 83 • 9 years ago

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