1285036 - Refactor some of the code in XMLHttpRequest

Status: RESOLVED FIXED

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

Description

[Thomas Wisniewski]

XMLHttpRequestMainThread could use a refactor, to not only make it cleaner and match the XHR2's wording more closely, but also get it into a shape where it can used as the only XHR implementation that's shared by workers and the main thread.

This bug will patch it step by side to bring it toward those goals, keeping each individual patch as simple as possible and self-contained (so tests don't break). Of course I'll also fix things that make more sense to fix in a given patch, rather than keeping existing buggy behavior.

Comment 1

[Thomas Wisniewski]

Attachment: 1285036 - part 1 - enum-and-constify-event-strings.diff

1 - Use an enum for XHR ProgressEvent names and use consts for a few other event strings.

Note that this is the same patch awaiting review in bug 918703, but it's a refactoring patch rather than one specific to that bug.

Comment 2

[Thomas Wisniewski]

Attachment: 1285036-2-use_an_enum_for_state_and_bools_for_flags.diff

2 - Split XMLHttpRequest.mState up into mState (an enum) and a series of mFlags (bools).

Comment 3

[Thomas Wisniewski]

Attachment: 1285036-2-flip_the_async_flag_to_match_spec.diff

3 - Invert mFlagAsynchronous as mFlagSynchronous, to match the spec's wording.

Comment 4

[Thomas Wisniewski]

Attachment: 1285036-4-remove-sent-pseudostate.diff

4 - Remove the "sent" pseudo-state and just add support for the spec's send() flag instead.

Comment 5

[Thomas Wisniewski]

Attachment: 1285036-5-consolidate_FireReadystatechange_code.diff

5 - Change CreateReadyStateChange() to FireReadyStateChange(), since it's not an event type that varies, and this is closer to the spec's wording.

Comment 6

[Thomas Wisniewski]

Attachment: 1285036-6-add_CString_ASCIIToLower_Upper.diff

6 - Add CString versions of ASCIIToLower/Upper, as ASCIIToLower will be needed soon, and I might as well add CString variants for both functions while I'm here.

Comment 7

[Thomas Wisniewski]

Attachment: 1285036-7-refactor_SetRequestHeader.diff

7 - Make SetRequestHeader() and related header code follow the spec much more closely. This means not actually setting the headers on the channel/request until the Send() method is called, but thankfully that only breaks one mochitest that doesn't require or test that non-standard behavior. In return, the code is more straightforward and a bunch of web platform tests now pass.

Comment 8

[Thomas Wisniewski]

Attachment: 1285036-8-refactor_Open.diff

8 - Make Open() follow the spec, passing more WPTs. I factor the code to initialize the channel out into its own method. I also have the various API endpoints for Open() all call the same internal _Open() method to keep things as consistent as possible.

Comment 9

[Thomas Wisniewski]

Attachment: 1285036-9-refactor_Send_endpoints_and_RequestBody.diff

9 - Refactor the Send() API endpoints, including the RequestBody class and GetRequestBody methods. Using template classes as in the patch should be cleaner and easier to manage without bloating the code. The Send(nsIVariant) endpoint also unwraps its argument right away and delegates it to the appropriate RequestBody type, simplifying that code a bit.

Comment 10

[Thomas Wisniewski]

Apologies for the review flood, :baku!

Successful try run for the first 9 patches (just two unrelated intermittents): https://treeherder.mozilla.org/#/jobs?repo=try&revision=66e3b44d3136

I'll continue chipping away at this...

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8768570 [details] [diff] [review]
1285036 - part 1 - enum-and-constify-event-strings.diff

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +103,5 @@
> +    NS_LITERAL_STRING("abort"),
> +    NS_LITERAL_STRING("timeout"),
> +    NS_LITERAL_STRING("load"),
> +    NS_LITERAL_STRING("loadend")
> +  };

Add static assertion about having ProgressEventTypeStrings and ProgressEventType always in sync.

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8768572 [details] [diff] [review]
1285036-2-use_an_enum_for_state_and_bools_for_flags.diff

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +1129,5 @@
>  
>    // If the state is UNSENT or OPENED,
>    // return the empty string and terminate these steps.
> +  if (mState == State::unsent ||
> +      mState == State::opened || mState == State::sent) {

Indent this better.

@@ +1194,5 @@
>    if (!httpChannel) {
>      // If the state is UNSENT or OPENED,
>      // return null and terminate these steps.
> +    if (mState == State::unsent ||
> +        mState == State::opened || mState == State::sent) {

here too.

@@ +3096,2 @@
>  {
> +  bool droppingFromSENTtoOPENED = mState == State::sent &&

Yeah.. I know. I have to finish the other review.

::: dom/xhr/XMLHttpRequestMainThread.h
@@ +698,1 @@
>  

I love this approach. Thanks for doing it. What I would like to see here is comments!
Can you write some documentation for each on of these flags?

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8768574 [details] [diff] [review]
1285036-4-remove-sent-pseudostate.diff

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +3001,5 @@
> +      return LOADING;
> +    case State::done:
> +      return DONE;
> +    default:
> +      NS_ASSERTION(false, "Unknown State!");

MOZ_CRASH("Unknown state");

::: dom/xhr/XMLHttpRequestMainThread.h
@@ +693,5 @@
>    bool mFlagHadUploadListenersOnSend;
>    bool mFlagACwithCredentials;
>    bool mFlagTimedOut;
>    bool mFlagDeleted;
> +  bool mFlagSend;

Document this as well.

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8768577 [details] [diff] [review]
1285036-7-refactor_SetRequestHeader.diff

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

This is patch 6.

Comment 15

[Thomas Wisniewski]

Attachment: 1285036-1-enum-and-constify-event-strings.diff

Added static assertion. Carrying over r=baku.

Comment 16

[Thomas Wisniewski]

Attachment: 1285036-2-use_an_enum_for_state_and_bools_for_flags.diff

Review comments addressed. Carrying over r+.

I added basic comments in the header for the states, as well as a link to the relevant part of the spec.

I'm definitely planning on documenting the various flags once I figure out which ones need to remain and why. I'm hoping to remove as many as possible, and make sure our non-spec behavior is as well-documented and factored out as I can get it.

Comment 17

[Thomas Wisniewski]

Attachment: 1285036-3-flip_the_async_flag_to_match_spec.diff

Re-attaching rebased version with correct filename (part 3, not 2). Carrying over r+.

Comment 18

[Thomas Wisniewski]

Attachment: 1285036-4-remove-sent-pseudostate.diff

Review comments addressed. Carrying over r+.

Comment 19

[Thomas Wisniewski]

Attachment: 1285036-4-remove-sent-pseudostate.diff

Actually carry over the r+ in the checkin comment this time...

Comment 20

[Thomas Wisniewski]

Attachment: 1285036-5-consolidate_FireReadystatechange_code.diff

Might as well rebase this while I'm here. Carrying over r+.

Comment 21

[Thomas Wisniewski]

Attachment: 1285036-7-refactor_SetRequestHeader.diff

>This is patch 6.

Huh. Not sure how that happened. Here's the real patch 7.

By the by, would it be alright to request check-in on (at least some of) the patches you've already reviewed, or should I hold off on doing that for now?

Comment 22

[Andrea Marchesini [:baku]]

Sure, just add the keyword "leave-open".

Comment 23

[Thomas Wisniewski]

Alright, thanks.

I'm requesting checkin for the reviewed patches, 1 to 6.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e94b77f805b
Part 1: Use an enum for ProgressEvent types and use consts for the event name strings. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/3036026722de
Part 2: Split mState up into an enum and a set of bool flags. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/b93efbaf1d7a
Part 3: Flip the asynchronous flag to synchronous, to match the spec. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0390a7afb12d
Part 4: Remove the 'sent' pseudostate and add support for the spec's send() flag instead. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae182434a6a
Part 5: Consolidate the readystatechange-firing code into a function named FireReadyStateChange(). r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7174077bd92
Part 6: Add CString variants of ASCIIToLower/Upper() for the next patch. r=baku

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Backed out for debug bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=31402431&repo=mozilla-inbound#L635

If you could, please also revise the commit messages similar to how the patches were landed (i.e. including the bug #, formatting cleanups, etc).

Comment 26

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f5af40eb365
Backed out 6 changesets for bustage on a CLOSED TREE.

Comment 27

[Thomas Wisniewski]

Attachment: 1285036-1-enum-and-constify-event-strings.diff

Indeed, I missed a debug assert that I shouldn't have missed: a good reminder for me to keep the debug mode flag turned on while I'm developing.

Removing that out-dated assert is all I'm changing in the related patch, so I don't suspect re-review is necessary. I'll also revise the commit messages... thanks for the heads-up about formatting!

Comment 28

[Thomas Wisniewski]

Attachment: 1285036-2-use_an_enum_for_state_and_bools_for_flags.diff

Comment 29

[Thomas Wisniewski]

Attachment: 1285036-3-flip_the_async_flag_to_match_spec.diff

Comment 30

[Thomas Wisniewski]

Attachment: 1285036-4-remove-sent-pseudostate.diff

Comment 31

[Thomas Wisniewski]

Attachment: 1285036-5-consolidate_FireReadystatechange_code.diff

Comment 32

[Thomas Wisniewski]

Attachment: 1285036-6-add_CString_ASCIIToLower_Upper.diff

Comment 33

[Thomas Wisniewski]

Alright, I've rebased the first 6 patches (with the aforementioned fix for debug breakage and the commit message changes). They should be good to go.

Comment 34

[Thomas Wisniewski]

Attachment: 1285036-10-refactor_request_header_handling.diff

Two more patches incoming. A try run for all the patches including both of these was successful (though with both of these new patches combined into one, before I split them up): https://treeherder.mozilla.org/#/jobs?repo=try&revision=afda7a56d9f5

This first patch mostly just pulls all the non-XHR-specific request header code into its own class named RequestHeader, to increase readability.

It also cleans up the chunk of logic in the Send() method related to picking the correct content-type/encoding for the request. It now matches spec step 4 more clearly.

Comment 35

[Thomas Wisniewski]

Attachment: 1285036-11-factor_channel_setup_code_out_of_send.diff

This patch factors all the Gecko specific channel-setup gunk out of the Send() method, so that it's easier to tell what it's really doing when it comes time to make it match the spec.

Note that I split the channel-setup code out into XHR-specific and non-XHR-specific functions. I suspect this will make it easier to unify it with the Fetch API's code, since the two APIs share the same "fetch request" logic in the spec.

Comment 36

[Thomas Wisniewski]

Attachment: 1285036-8-refactor_Open.diff

Also here's a new patch #8 which fixes a couple of debug assertions in the worker XHR code that don't expect a couple of changes related to spec-alignment.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e42827ec6521
Part 1: Use an enum for ProgressEvent types and use consts for the event name strings. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c7274bdcd99
Part 2: Split mState up into an enum and a set of bool flags. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf25c8b6af9f
Part 3: Flip the asynchronous flag to synchronous, to match the spec. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/18a736ed2156
Part 4: Remove the 'sent' pseudostate and add support for the spec's send() flag instead. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4fc0549bed0
Part 5: Consolidate the readystatechange-firing code into a function named FireReadyStateChange(). r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/21cd5236715f
Part 6: Add CString variants of ASCIIToLower/Upper() for the next patch. r=baku

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e42827ec6521
https://hg.mozilla.org/mozilla-central/rev/9c7274bdcd99
https://hg.mozilla.org/mozilla-central/rev/bf25c8b6af9f
https://hg.mozilla.org/mozilla-central/rev/18a736ed2156
https://hg.mozilla.org/mozilla-central/rev/d4fc0549bed0
https://hg.mozilla.org/mozilla-central/rev/21cd5236715f

Comment 39

[Thomas Wisniewski]

Attachment: 1285036-7-refactor_SetRequestHeader.diff

Rebased patch 7.

Comment 40

[Thomas Wisniewski]

Attachment: 1285036-9-refactor_Send_endpoints_and_RequestBody.diff

Rebased patch 9.

Comment 41

[Thomas Wisniewski]

Attachment: 1285036-10-refactor_request_header_handling.diff

Rebased patch 10.

Comment 42

[Thomas Wisniewski]

Attachment: 1285036-11-factor_channel_setup_code_out_of_send.diff

Rebased patch 11.

Comment 43

[Thomas Wisniewski]

A fresh try-run for rebased patches 7-11 seems ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b0be446311a

Comment 44

[Andrea Marchesini [:baku]]

Comment on attachment 8771700 [details] [diff] [review]
1285036-7-refactor_SetRequestHeader.diff

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +3159,5 @@
> +XMLHttpRequestMainThread::GetAuthorRequestHeaderValue(const char* aName,
> +                                                      nsACString& outValue)
> +{
> +  for (RequestHeader& header : mAuthorRequestHeaders) {
> +    if (header.name.Equals(aName)) {

EqualsLiteral

Comment 45

[Andrea Marchesini [:baku]]

Comment on attachment 8769548 [details] [diff] [review]
1285036-8-refactor_Open.diff

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +1366,2 @@
>    }
> +  return _Open(aMethod, aUrl, async, username, password);

call it OpenInternal

@@ +1455,5 @@
> +    parsedURL->GetHost(host);
> +    if (!host.IsEmpty()) {
> +      nsAutoCString userpass;
> +      if (aUsername.WasPassed()) {
> +        CopyUTF16toUTF8(aUsername.Value(), userpass); // todo: percent-escape as per spec

Are you going to do something here as well?

@@ +1459,5 @@
> +        CopyUTF16toUTF8(aUsername.Value(), userpass); // todo: percent-escape as per spec
> +      }
> +      userpass.AppendLiteral(":");
> +      if (aPassword.WasPassed()) {
> +        AppendUTF16toUTF8(aPassword.Value(), userpass); // todo: percent-escape as per spec

same here.

@@ +1483,1 @@
>    mFlagSend = false;

Why this is here and not it's part of step 11 ?

::: dom/xhr/XMLHttpRequestMainThread.h
@@ +588,5 @@
>    void StartProgressEventTimer();
>  
>    nsresult OnRedirectVerifyCallback(nsresult result);
>  
> +  nsresult _Open(const nsACString& aMethod, const nsACString& aUrl,

OpenInternal

Comment 46

[Andrea Marchesini [:baku]]

Comment on attachment 8771713 [details] [diff] [review]
1285036-9-refactor_Send_endpoints_and_RequestBody.diff

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +2140,5 @@
> +
> +    // Make sure to use the encoding we'll send
> +    rv = serializer->SerializeToStream(domdoc, output, aCharset);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +

extra line

@@ +2150,5 @@
> +  rv = storStream->GetLength(&length);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  *aContentLength = length;
> +
> +  return storStream->NewInputStream(0, aResult);

rv = storStream...
NS_ERROR_FAILURE(rv, rv);

return NS_OK;

@@ +2163,5 @@
> +  aCharset.AssignLiteral("UTF-8");
> +
> +  nsCString converted = NS_ConvertUTF16toUTF8(*mBody);
> +  *aContentLength = converted.Length();
> +  return NS_NewCStringInputStream(aResult, converted);

same here

@@ +2176,5 @@
> +  aCharset.Truncate();
> +
> +  nsresult rv = mBody->Available(aContentLength);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +

nsCOMPtr<nsIInputStream> stream(mBody);
stream.forget(aResult);

return NS_OK;

::: dom/xhr/XMLHttpRequestMainThread.h
@@ +243,5 @@
>    public:
> +    virtual nsresult GetAsStream(nsIInputStream** aResult,
> +                                 uint64_t* aContentLength,
> +                                 nsACString& aContentType,
> +                                 nsACString& aCharset) const

virtual nsresult GetAsStream(nsIInputStream** aResult,
                                 uint64_t* aContentLength,
                                 nsACString& aContentType,
                                 nsACString& aCharset) const = 0; ?

@@ +251,5 @@
> +    }
> +  };
> +
> +  template<typename Type>
> +  class RequestBody : public RequestBodyBase

final

@@ +266,3 @@
>    };
>  
> +  nsresult _Send(const RequestBodyBase* aBody);

SendInternal

Comment 47

[Thomas Wisniewski]

> EqualsLiteral

I tried that already, but it doesn't compile with EqualsLiteral (GC 5.4.0):

>if (header.name.EqualsLiteral(aName)) {
>error: mismatched types ‘const char [N]’ and ‘const char*’



> > +        CopyUTF16toUTF8(aUsername.Value(), userpass); // todo: percent-escape as per spec
> Are you going to do something here as well?

I'll remove these comments and file a follow-up bug, since this bug doesn't have to fix everything.



> >    mFlagSend = false;
> Why this is here and not it's part of step 11 ?

No real reason. I'll move it down a few lines.



I'll also make the other changes you suggest.

Comment 48

[Thomas Wisniewski]

Attachment: 1285036-7-refactor_SetRequestHeader.diff

Incoming updated versions of patches 7-9, addressing the review comments and carrying over r+.

A try run with all 3 still seems clear: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1410cfdc028d

Comment 49

[Thomas Wisniewski]

Attachment: 1285036-8-refactor_Open.diff

Comment 50

[Thomas Wisniewski]

Attachment: 1285036-9-refactor_Send_endpoints_and_RequestBody.diff

Comment 51

[Thomas Wisniewski]

Requesting checkin of patches 7, 8 and 9.

Comment 52

[Carsten Book [:Tomcat]]

part 8 has problems like 2 out of 2 hunks FAILED -- saving rejects to file dom/xhr/XMLHttpRequestWorker.cpp.rej
unable to find 'testing/web-platform/meta/XMLHttpRequest/open-open-send.htm.ini' for patching
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/meta/XMLHttpRequest/open-open-send.htm.ini.rej
unable to find 'testing/web-platform/meta/XMLHttpRequest/open-open-sync-send.htm.ini' for patching
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/meta/XMLHttpRequest/open-open-sync-send.htm.ini.rej
unable to find 'testing/web-platform/meta/XMLHttpRequest/open-send-open.htm.ini' for patching
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/meta/XMLHttpRequest/open-send-open.htm.ini.rej
unable to find 'testing/web-platform/meta/XMLHttpRequest/open-sync-open-send.htm.ini' for patching
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/meta/XMLHttpRequest/open-sync-open-send.htm.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1285036-8-refactor_Open.diff

Comment 53

[Thomas Wisniewski]

My bad, I wasn't paying enough attention to whether my checkin-requests would clash. I'll slow it down.

Comment 54

[Thomas Wisniewski]

Attachment: 1285036-7-refactor_SetRequestHeader.diff

Rebased patches incoming, carrying over r+ for 7-9.

Comment 55

[Thomas Wisniewski]

Attachment: 1285036-8-refactor_Open.diff

Comment 56

[Thomas Wisniewski]

Attachment: 1285036-9-refactor_Send_endpoints_and_RequestBody.diff

Comment 57

[Thomas Wisniewski]

Attachment: 1285036-10-refactor_request_header_handling.diff

Comment 58

[Thomas Wisniewski]

Attachment: 1285036-11-factor_channel_setup_code_out_of_send.diff

Comment 59

[Thomas Wisniewski]

A fresh try run for the rebased patches still seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=62475446d2ff

Re-requesting checkin for patches 7, 8 and 9.

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16fefebdbb50
Part 7: Change SetRequestHeader() and related header code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/abbef296a82f
Part 8: Change XHR open() and related code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a00db57d77a
Part 9: Clean up the XHR send() API endpoints, and how nsIVariants are handled. r=baku

Comment 61

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=32297699&repo=mozilla-inbound

Comment 62

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5695eb78dcdc
Backed out changeset 8a00db57d77a 
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f9e16f3e5a
Backed out changeset abbef296a82f 
https://hg.mozilla.org/integration/mozilla-inbound/rev/316baefffba9
Backed out changeset 16fefebdbb50 for static build bustage

Comment 63

[Thomas Wisniewski]

Ugh. I'll bet it's my old friend "explicit" to the rescue once again. Doing a try-run now, hope I picked the right machine on the TryChooser Syntax Builder...

Thanks Tomcat!

Comment 64

[Thomas Wisniewski]

Attachment: 1285036-9-refactor_Send_endpoints_and_RequestBody.diff

Patch 9, now with more explicit. Carrying over r+.

Comment 65

[Thomas Wisniewski]

Attachment: 1285036-10-refactor_request_header_handling.diff

Patch 10, now with more explicit. Carrying over r+.

Comment 66

[Thomas Wisniewski]

Requesting checkin once again for patches 7, 8, 9. Hopefully they'll stick this time.

Try showing the static-check success: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a3c4c26de40

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b51b69f723e
Part 7: Change SetRequestHeader() and related header code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/6462cd2ea249
Part 8: Change XHR open() and related code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd18c03c696
Part 9: Clean up the XHR send() API endpoints, and how nsIVariants are handled. r=baku

Comment 68

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_net_resend.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=32395831&repo=mozilla-inbound#L4718

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2508db54956

Comment 69

[Ryan VanderMeulen [:RyanVM]]

And browser_net_copy_as_curl.js.
https://treeherder.mozilla.org/logviewer.html#?job_id=32392590&repo=mozilla-inbound#L10279

Comment 70

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thomas, is there a chance that this work would fix bug 827243?

Comment 71

[Thomas Wisniewski]

Thanks Ryan, I'll investigate the failures.

@gandalf:
>is there a chance that this work would fix bug 827243?

It seems doubtful, based on bz's comments in that bug. I'm trying to limit this refactor just to the XHR (and possibly fetch) code, while leaving necko's code alone if possible.

I don't mind investigating that bug when I have some time, though.

Comment 72

[Thomas Wisniewski]

Attachment: 1285036-7-refactor_SetRequestHeader.diff

Alright, here's a new patch 7 which fixes the issue with browser_net_resend.js. A try run shows those bugs are gone, and only things that seem truly unrelated are showing up: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0abe754cf0e8

The changes I had to make were again nothing I'd expect needs a review; I just had to compensate for the tests expecting header names from XHRs to be a specific case, when they're really not case-sensitive fields.

While here, I also modified the curl test so that it doesn't expect the shell command it gets to have its parameters in a particular order, since their order doesn't matter (and my local system gets them in a slightly different order than the try servers do, for whatever reason).

At any rate, I'm once again requesting check-in of patches 7, 8 and 9, since things seem clear.

Comment 73

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41a51d368f38
Part 7: Change SetRequestHeader() and related header code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/de078c2e0991
Part 8: Change XHR open() and related code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6d55b02d19
Part 9: Clean up the XHR send() API endpoints, and how nsIVariants are handled. r=baku

Comment 74

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Had to back this out for failing browser_net_copy_as_curl.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f2108bbcb76
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2ad5d36342
https://hg.mozilla.org/integration/mozilla-inbound/rev/d30601c6f4e4

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=32807593&repo=mozilla-inbound

Comment 75

[Thomas Wisniewski]

Wow is that test ever fragile. I'll have to run it through every machine on try before I try to commit it again, just in case.

Thanks for the backout!

Comment 76

[Andrea Marchesini [:baku]]

Comment on attachment 8772986 [details] [diff] [review]
1285036-11-factor_channel_setup_code_out_of_send.diff

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

r- mainly for the use of nsCOMPtr. Plus, what about lower/upper case comparison of methods?
I'll be in PTO for 2 weeks. In the meantime, maybe you could send review requests to smaug. Ask him first. He is often very busy!
Thanks for the awesome work!

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +2274,2 @@
>  nsresult
> +CreateChannelForFetch(nsCOMPtr<nsIChannel>& aChannel,

nsIChannel** aChannel

@@ +2274,5 @@
>  nsresult
> +CreateChannelForFetch(nsCOMPtr<nsIChannel>& aChannel,
> +                      const nsCOMPtr<nsILoadGroup>& aLoadGroup,
> +                      const nsCOMPtr<nsIPrincipal>& aPrincipal,
> +                      const nsCOMPtr<nsIDocument>& aResponsibleDocument,

just pass the pointers.

@@ +2312,4 @@
>    nsresult rv;
> +  if (aResponsibleDocument &&
> +      aResponsibleDocument->NodePrincipal() == aPrincipal) {
> +    rv = NS_NewChannel(getter_AddRefs(aChannel),

rv = NS_NewChannel(aChannel, ...

@@ +2321,5 @@
>                         nullptr,   // aCallbacks
>                         loadFlags);
>    } else {
>      // Otherwise use the principal.
> +    rv = NS_NewChannel(getter_AddRefs(aChannel),

rv = NS_NewChannel(aChannel, ...

@@ +2332,5 @@
>                         loadFlags);
>    }
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(aChannel));

nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(*aChannel));

@@ +2373,5 @@
>  }
>  
> +// Non-XMLHttpRequest-specific channel setup goes here.
> +nsresult
> +SetupChannelForFetch(const nsCOMPtr<nsIChannel>& aChannel,

Ditto.

@@ +2721,2 @@
>    if (aBody && httpChannel &&
> +      !mRequestMethod.EqualsLiteral("GET") &&

do we convert this in upper case somewhere else?

::: dom/xhr/XMLHttpRequestMainThread.h
@@ +234,5 @@
>    // states
>    virtual uint16_t ReadyState() const override;
>  
>    // request
> +  nsresult Fetch(nsCOMPtr<nsIInputStream>& aUploadStream, int64_t aUploadLength);

nsIInputStream** aUploadStream

@@ +238,5 @@
> +  nsresult Fetch(nsCOMPtr<nsIInputStream>& aUploadStream, int64_t aUploadLength);
> +
> +  nsresult CreateChannelForFetch();
> +
> +  nsresult SetupChannelForFetch(const nsCOMPtr<nsIChannel>& aChannel,

nsIChannel* aChannel, nsPIDOMWindowInner* aOwner, nsIPrincipal* aPrincipal, .... nsIInputStream** aUploadStream

Comment 77

[Andrea Marchesini [:baku]]

Comment on attachment 8773302 [details] [diff] [review]
1285036-10-refactor_request_header_handling.diff

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

Smaug, can you take this?

Comment 78

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8773302 [details] [diff] [review]
1285036-10-refactor_request_header_handling.diff


>+++ b/dom/xhr/XMLHttpRequestMainThread.cpp
>@@ -2458,19 +2458,16 @@ XMLHttpRequestMainThread::SendInternal(c
>   //     an asynchronous call.
> 
>   // Ignore argument if method is GET, there is no point in trying to
>   // upload anything
>   nsAutoCString method;
>   nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(mChannel));
> 
>   if (httpChannel) {
>-    // Spec step 5
>-    SetAuthorRequestHeadersOnChannel(httpChannel);
I don't understand this change. Please explain.

>-    nsAutoCString charset;
>-    nsAutoCString defaultContentType;
>+    nsAutoCString contentType;
>+    contentType.SetIsVoid(true);
>+
>+    nsAutoCString encoding;
>+    encoding.SetIsVoid(true);
>+
Why the SetIsVoid calls?
You pass contentType and encoding to GetAsStream anyhow, so it should set the values to something reasonable when
success value is returned, I think. Though, I don't know what GetAsStream does. It is not in the tree right now. Does some patch here add it?


>+public:
>+  class CharsetIterator {
Nit, { goes to its own line

This is a bit complicated (largely because I haven't reviewed other related patches ,I think). I need to re-read.

But please fix those and explain and ask review again.

Comment 79

[Thomas Wisniewski]

Attachment: 1285036-7-refactor_SetRequestHeader.diff

Thanks for the reviews, guys, I'll address the comments for those patches as soon as I can.

For now, I'm at least ready to retry landing the already-reviewed patches. New versions of 7 and 9 are incoming (8 was still fine). Carrying over r+, as there are no functional changes, just updates to the aforementioned test to make it less fragile.

A try run hasn't yet revealed anything worrying (though I can't seem to get try to run WinXP tests, it stalls forever... even the Win8 debug ones spend many hours in a queue): https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8f330e1fbe9

Comment 80

[Thomas Wisniewski]

Attachment: 1285036-9-refactor_Send_endpoints_and_RequestBody.diff

And here's the new patch 9.

Requesting check-in again for patches 7, 8 and 9.

Comment 81

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15171e9168c4
Part 7: Change SetRequestHeader() and related header code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd7ab32846c2
Part 8: Change XHR open() and related code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a87074952ee
Part 9: Clean up the XHR send() API endpoints, and how nsIVariants are handled. r=baku

Comment 82

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15171e9168c4
https://hg.mozilla.org/mozilla-central/rev/dd7ab32846c2
https://hg.mozilla.org/mozilla-central/rev/4a87074952ee

Comment 84

[Thomas Wisniewski]

Attachment: 1285036-10-refactor_request_header_handling.diff

(In reply to Olli Pettay [:smaug] from comment #78)

Here's a rebased version of the patch.


> >   if (httpChannel) {
> >-    // Spec step 5
> >-    SetAuthorRequestHeadersOnChannel(httpChannel);
> I don't understand this change. Please explain.

I essentially just moved this line down to where the rest of spec step 5 (the actual request-fetch) is done. Sorry that wasn't clearer.


> Why the SetIsVoid calls?
> You pass contentType and encoding to GetAsStream anyhow, so it should set
> the values to something reasonable when
> success value is returned, I think.

I figured it would be safer to just have the GetAsStream methods assume they only have to set the values if they need to, leaving them null otherwise. This also matches the spec's current logic/wording (for send step 4), but I certainly don't mind changing it.


>Though, I don't know what GetAsStream does. It is not in
> the tree right now. Does some patch here add it?

Yes, this was from the previous patch in the set (#9), which has since landed. Essentially, I factored out the logic to get the request body stream into a series of template classes, RequestBody<type>, each with a GetAsStream method.


> >+public:
> >+  class CharsetIterator {
> Nit, { goes to its own line

Done.


> This is a bit complicated (largely because I haven't reviewed other related
> patches ,I think). I need to re-read.

Sure, no rush. If you'd like I can lurk on an IRC channel of your choice in case you'd like more direct feedback.

Comment 85

[Thomas Wisniewski]

Attachment: 1285036-11-factor_channel_setup_code_out_of_send.diff

Actually sorry, the patch you were looking at was this one I'm updating with this comment (I've updated both 10 and 11).

The patch in my previous comment addresses :baku's concerns in comment 76. In answer to his question about case-sensitivity, it shouldn't be an issue because the request method has already been case-normalized in the open() method as per spec (in a previous patch that's already landed).

Comment 86

[Thomas Wisniewski]

Oh, wait... you actually *were* reviewing patch 10, baku had just skipped to 11. My bad, sorry for any undue confusion.

Comment 87

[Jarda Snajdr [:jsnajdr]]

Hello Thomas, I'm working on devtools bug 1269102 and bug 1270096 and I have a few questions about the changes you did in the "part 7" patch:

1. When I do xhr.setRequestHeader("X-Custom", "value"), the header name is converted to lowercase "x-custom" and send as such. Why this change? Is it because of some recent change in the spec, namely [1]? I don't see any other browsers implementing this (Chrome, Safari), so I'm a bit confused.

2. Why the EXPECTED_RESULT.toString() call at [2]? EXPECTED_RESULT is already a string.

3. By the way, the set-matching algorithm in browser_net_copy_as_curl.js doesn't check the "--compressed" parameter (added when Accept-Encoding header is encountered). I'll fix that as part of bug 1269102.

[1] https://fetch.spec.whatwg.org/#concept-header-list-combine
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_copy_as_curl.js#67

Comment 88

[Thomas Wisniewski]

1. Yes, the change was made to match the spec (and HTTP header names aren't supposed to be case-sensitive to begin with). But if this is causing compatibility issues, then I can change it back. It would be best for us to raise any issues with the spec authors, though.

2. Good point. I just forgot to remove it. If it works without the toString, then feel free to remove it while you update the code.

3. Thanks!

Comment 89

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Thomas Wisniewski from comment #88)
> 1. Yes, the change was made to match the spec (and HTTP header names aren't
> supposed to be case-sensitive to begin with). But if this is causing
> compatibility issues, then I can change it back. It would be best for us to
> raise any issues with the spec authors, though.
If others aren't doing that, I think we should revert the change, and get the spec fixed.
And if the patch doing that is already in branches, better to make sure it is backed out also there.
So, file a separate bug to deal all that.


Did some of the patches land before the previous merge? If some patches in a bug land for version X and some others X+1, that makes the regression hunting and tracking way more difficult. 
So better to split stuff to separate bugs in the future.
Not sure what to do with this bug though.

Comment 90

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Thomas Wisniewski from comment #84)
> > Why the SetIsVoid calls?
> > You pass contentType and encoding to GetAsStream anyhow, so it should set
> > the values to something reasonable when
> > success value is returned, I think.
> 
> I figured it would be safer to just have the GetAsStream methods assume they
> only have to set the values if they need to, leaving them null otherwise.
> This also matches the spec's current logic/wording (for send step 4), but I
> certainly don't mind changing it.
So is this changing the behavior? refactoring shouldn't change behavior, such changes should happen in separate bugs.

> Sure, no rush. If you'd like I can lurk on an IRC channel of your choice in
> case you'd like more direct feedback.
I'm in #content and #developers at least

Comment 91

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8779054 [details] [diff] [review]
1285036-10-refactor_request_header_handling.diff

>     // If the user hasn't overridden the Accept header, set it to */* as per spec
>-    nsAutoCString acceptHeader;
>-    GetAuthorRequestHeaderValue("accept", acceptHeader);
>-    if (acceptHeader.IsVoid()) {
>-      httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Accept"),
>-                                    NS_LITERAL_CSTRING("*/*"),
>-                                    false);
>+    if (!mAuthorRequestHeaders.Has("accept")) {
>+      mAuthorRequestHeaders.Set("accept", NS_LITERAL_CSTRING("*/*"));
So the old code sets "Accept", new one "accept". No such changes in refactoring patches please.



Sorry, I will need to read this still couple of times to ensure there are no functionality changes.

Comment 92

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8779055 [details] [diff] [review]
1285036-11-factor_channel_setup_code_out_of_send.diff


+// Non-XMLHttpRequest-specific channel init goes here.
 nsresult
-XMLHttpRequestMainThread::InitChannel()
+CreateChannelForFetch(nsIChannel** aChannel,
+                      const nsCOMPtr<nsILoadGroup>& aLoadGroup,
+                      const nsCOMPtr<nsIPrincipal>& aPrincipal,
+                      const nsCOMPtr<nsIDocument>& aResponsibleDocument,
+                      const nsACString& aMethod,
+                      nsCOMPtr<nsIURI>& aURL,
+                      bool aIsMozAnon,
+                      bool aIsPrivileged)
I don't understand the reason for this method, which is just called in otherwise small
XMLHttpRequestMainThread::CreateChannelForFetch().
Merge them. And if we later want to have a generic CreateChannelForFetch, do that split in a separate bug.


>-  nsresult InitChannel();
>+  nsresult Fetch(nsIInputStream* aUploadStream, int64_t aUploadLength);
Fetch as a method name makes one think something happens synchronously if there is more work happening after Fetch() call, like there is in 
XMLHttpRequestMainThread::SendInternal.
Rename Fetch to InitiateFetch or some such.

>+
>+  nsresult CreateChannelForFetch();
>+
>+  nsresult SetupChannelForFetch(nsIChannel* aChannel,
>+                                nsPIDOMWindowInner* aOwner,
>+                                nsIPrincipal* aPrincipal,
>+                                nsACString& aMethod,
>+                                RequestHeaders& aHeaders,
>+                                nsIInputStream* aUploadStream,
>+                                int64_t aUploadLength,
>+                                bool aIsPrivileged,
>+                                bool aIsMozAnon,
>+                                bool aWithCredentials,
>+                                bool aForcePreflight,
>+                                bool aWantProgressNotifications);
Not sure why these methods have Fetch in their name. Wouldn't CreateChannel and SetupChannel be simpler and easier to understand.

Comment 93

[Thomas Wisniewski]

>So is this changing the behavior?

No, those parts weren't about behavioral changes, just refactoring. The patch that caused the header case-sensitivity issue was #7, and I'm currently working on a patch to fix that bit (and any other behavioral change I may have introduced in that patchset).


>Did some of the patches land before the previous merge?

Yes, patches 1-9 have already landed in several batches. At this point it will be easiest to revert the changes manually, as my memory is still fresh regarding these changes.


>Sorry, I will need to read this still couple of times to ensure there are no functionality changes.

No, it's my bad for mixing refactoring with behavioral changes. After fixing the above, I'll take another look at these two unlanded patches again as well.


>I don't understand the reason for this method

That was just to pull out the XHR-specific channel-setting bits from the more general stuff. I'll merge those two methods together as you suggest.


>Rename Fetch to InitiateFetch or some such.

Will do.


>Not sure why these methods have Fetch in their name.

Sure, I'll rename them as you recommend.


Thanks for the review effort, I'll also try my best to keep any subsequent patches as small as I can. I hope the next big part (converting send() to using the Fetch driver in /dom/fetch) won't be a horrible nightmare...

Comment 94

[Thomas Wisniewski]

Attachment: 1285036-10-factor-request-header-code-into-its-own-class.diff

Alright, I've set up bug 1293881 to deal with reverting the behavioral change to header-name casing introduced in patch 7.

I've also changed patch 10 to simply be about factoring the request header code into its own class, rather than also trying to match the spec in the send() method. That should hopefully make it more straightforward to review, as it's basically just moving around code (the accept=*/* code included). Here's hoping the CharsetIterator stuff isn't too subtle!

(I'm not sure whether setting r+ again is necessary, so I apologize in advance if it's a nuisance at this point.)

Comment 95

[Thomas Wisniewski]

Attachment: 1285036-11-factor-fetch-code-out-into-InitiateFetch.diff

Here's a new version of patch 11. Hopefully this one will be easier to review. It's still just moving code around, but I agree that there's no real point in having so many channel-setup methods. This version just moves all that stuff into InitiateFetch instead. Only minor changes to the moved code were made to accommodate that, rename a couple of things, reformat a few lines for 80 chars, and add/update a couple of comments.

For the change that involves it, bear in mind that mRequestMethod uppercases GET/POST and other values when it's set in the Open method (which of course happens well before Send calls the relevant code).

Comment 96

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8779602 [details] [diff] [review]
1285036-10-factor-request-header-code-into-its-own-class.diff

>exporting patch:
># HG changeset patch
># User Thomas Wisniewski <[e-mail]>
># Date 1470797759 14400
>#      Tue Aug 09 22:55:59 2016 -0400
># Node ID c86d824d9ed027636f9c4378515e6afe4ec1dd34
># Parent  c29c58b018ec75ca8d9a44f08736ef58e1e3855a
>Bug 1285036 - Part 10: Factor the request-header XHR code out into its own class.
>
>diff --git a/dom/xhr/XMLHttpRequestMainThread.cpp b/dom/xhr/XMLHttpRequestMainThread.cpp
>--- a/dom/xhr/XMLHttpRequestMainThread.cpp
>+++ b/dom/xhr/XMLHttpRequestMainThread.cpp
>@@ -2525,37 +2525,33 @@ XMLHttpRequestMainThread::SendInternal(c
>   //     an asynchronous call.
> 
>   // Ignore argument if method is GET, there is no point in trying to
>   // upload anything
>   nsAutoCString method;
>   nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(mChannel));
> 
>   if (httpChannel) {
>+    // If the user hasn't overridden the Accept header, set it to */* as per spec
>+    if (!mAuthorRequestHeaders.Has("accept")) {
>+      mAuthorRequestHeaders.Set("Accept", NS_LITERAL_CSTRING("*/*"));
>+    }
>+
>     // Spec step 5
>-    SetAuthorRequestHeadersOnChannel(httpChannel);
>+    mAuthorRequestHeaders.ApplyToChannel(httpChannel);
Doesn't this end up changing the ordering of headers? Accept might be there earlier than without the patch, right?
Is this change compatible what other browsers do?

Actually, I don't see anything in Fetch spec which says implicit "Accept" should be applied before author headers. 
Fix, or explain where the spec requires this ordering

Comment 97

[Thomas Wisniewski]

Nope. If I perform this XHR:

  var client = new XMLHttpRequest()
  client.open("GET", "", false)
  client.setRequestHeader("X-junk", "x-some-value")
  client.send(null)
  assert_equals(client.responseText, "*/*")
  client.open("GET", "", false)

Here are the headers I get with my patch:

  Host: localhost:8001
  User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
  Accept: */*
  Accept-Language: en-US,en;q=0.5
  Accept-Encoding: gzip, deflate
  Referer: http://localhost:8001/
  x-junk: x-some-value

Here are the headers I get without my patch:

  Host: localhost:8001
  User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
  Accept: */*
  Accept-Language: en-US,en;q=0.5
  Accept-Encoding: gzip, deflate
  Referer: http://localhost:8001/
  x-junk: x-some-value

And here are the headers from both versions when I also setRequestHeader("accept", "whatever"):

  Host: localhost:8080
  User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
  Accept: whatever
  Accept-Language: en-US,en;q=0.5
  Accept-Encoding: gzip, deflate
  Referer: http://localhost:8080/
  x-junk: x-something/vague, text/html5

So it seems necko always emits the Accept header in the same place regardless.

Comment 98

[Thomas Wisniewski]

I should also say that I don't mind omitting that change and just continuing to just set the header on the channel directly. Either way is fine, so I'll update the patch accordingly as your review progresses.

Comment 99

[Olli Pettay [:smaug][[e-mail]]]

ok, thanks for testing. If necko anyhow orders the headers somehow, then fine.

Comment 100

[Olli Pettay [:smaug][[e-mail]]]

But, any reason why you order header setting differently to what the spec does?

Comment 101

[Thomas Wisniewski]

No, but I'm not 100% sure what you mean. The XHR spec actually wants us to call the internal Fetch API to actually make the channel and set the headers, but we're doing things so much differently that I doubt we match the spec in many respects. Still, if you feel I could move those lines to another spot to kinda match the spec a bit better, I certainly don't mind doing so.

(Incidentally, I'm working on a proof-of-concept patch to see whether we could just switch over to using the Fetch driver in this code, with minimal changes/wrappers over that API as necessary for compat... but of course that's a whole other can of worms).

Comment 102

[Olli Pettay [:smaug][[e-mail]]]

I mean, per spec author headers are added first and then "Accept", if author headers didn't contain it, but for some reason you do it in different order.

Comment 103

[Thomas Wisniewski]

Oh, I see. No, my code will still end up adding accept after the user's specified headers. It only adds the Accept header if it isn't there to begin with, which will ultimately place at the end of RequestHeaders.mHeaders (an array). ApplyToChannel will then apply them in array order, so it will be set last.

Still, if this is reading that subtly then I'd rather not make the change at all, and just keep it overt (that is, just do mAuthorRequestHeaders.ApplyToChannel, and then consider whether Accept must be set with a subsequent channel.SetRequestHeader).

Comment 104

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8779602 [details] [diff] [review]
1285036-10-factor-request-header-code-into-its-own-class.diff

>+void
>+RequestHeaders::Get(const nsACString& aName, nsACString& outValue) const
>+{
Why you don't use Find to implement Get?
RequestHeader* header = Find(aName);
if (header) {
  aValue = header->mValue;
} else {
  aValue.SetIsVoid(true);
}




>+RequestHeaders::Merge(const char* aName, const nsACString& aValue)
>+{
>+  Merge(nsDependentCString(aName), aValue);
>+}
>+
>+void
>+RequestHeaders::Merge(const nsACString& aName, const nsACString& aValue)
>+{
>+  RequestHeader* header = Find(aName);
>+  if (header) {
>+    header->value.AppendLiteral(", ");
>+    header->value.Append(aValue);
>+  } else {
>+    RequestHeader newHeader = {
>+      nsCString(aName), nsCString(aValue)
>+    };
>+    mHeaders.AppendElement(newHeader);
>+  }
>+}
Could you rename both Merge methods to MergeOrSet or some such
since just Merge isn't clear what happens when there isn't old value.



>+RequestHeaders::CharsetIterator::Next()
>+{
>+  int32_t start, end;
>+  nsAutoCString charset;
>+  NS_ExtractCharsetFromContentType(Substring(mSource, 0, mCutoff),
>+                                   charset, &mValid, &start, &end);
>+
>+  if (!mValid) {
>+    return false;
>+  }
>+
>+  // Everything after the = sign is the part of the charset we want.
>+  mCurPos = mSource.FindChar('=', start) + 1;
>+  mCurLen = end - mCurPos;
>+
>+  // Special case: the extracted charset is quoted with single quotes.
>+  // For the purpose of preserving what was set we want to handle them
>+  // as delimiters (although they aren't really).
>+  if (charset.Length() >= 2 &&
>+      charset.First() == '\'' &&
>+      charset.Last() == '\'') {
>+    ++mCurPos;
>+    mCurLen -= 2;
>+  }
>+
>+  mCutoff = start;
So this new code misses the rather critical comment how mCutoff is used.
The old code has 
"    ... limiting the
  // search to only look for charsets before the current charset, to
  // prevent finding the same charset twice."
Please keep that comment in some form



>+class RequestHeaders
>+{
>+  struct RequestHeader
>+  {
>+    nsCString name;
>+    nsCString value;
mName and mValue
new code should use the normal coding style.


>+  bool Has(const char* aName) const;
>+  bool Has(const nsACString& aName) const;
>+  void Get(const char* aName, nsACString& outValue) const;
aValue


>+  void Get(const nsACString& aName, nsACString& outValue) const;
aValue


>+  void GetCORSUnsafeHeaders(nsTArray<nsCString>& outArray) const;
aArray

Also method definitions should use aFoo naming

Comment 105

[Olli Pettay [:smaug][[e-mail]]]

That accept handling can be as it is in the patch.

Comment 106

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8779796 [details] [diff] [review]
1285036-11-factor-fetch-code-out-into-InitiateFetch.diff

ok, mostly code move.

Comment 107

[Thomas Wisniewski]

Attachment: 1285036-10-factor-request-header-code-into-its-own-class.diff

>Why you don't use Find to implement Get?

Just an oversight. I've changed it to do so.


>Could you rename both Merge methods to MergeOrSet
>Please keep that comment in some form
>new code should use the normal coding style.

Done.


Here's an updated patch with those changes.

Comment 108

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8779924 [details] [diff] [review]
1285036-10-factor-request-header-code-into-its-own-class.diff

>+RequestHeaders::Find(const nsACString& aName)
>+{
>+  const nsCaseInsensitiveCStringComparator ignoreCase;
>+  for (RequestHeaders::RequestHeader& header : mHeaders) {
>+    if (header.mName.Equals(aName, ignoreCase)) {
>+      return &header;
>+    }
>+  }
>+  return nullptr;
>+}
>+
>+bool
>+RequestHeaders::Has(const char* aName) const
>+{
>+  return Has(nsDependentCString(aName));
>+}
>+
>+bool
>+RequestHeaders::Has(const nsACString& aName) const
>+{
>+  const nsCaseInsensitiveCStringComparator ignoreCase;
>+  for (const RequestHeader& header : mHeaders) {
>+    if (header.mName.Equals(aName, ignoreCase)) {
>+      return true;
>+    }
>+  }
>+  return false;
Couldn't 'Has' actually be just
  return !!Find(aName);


>+RequestHeaders::CharsetIterator::Next()
>+{
>+  int32_t start, end;
>+  nsAutoCString charset;
>+  NS_ExtractCharsetFromContentType(Substring(mSource, 0, mCutoff),
>+                                   charset, &mValid, &start, &end);
>+
>+  if (!mValid) {
>+    return false;
>+  }
>+
>+  // Everything after the = sign is the part of the charset we want.
>+  mCurPos = mSource.FindChar('=', start) + 1;
>+  mCurLen = end - mCurPos;
>+
>+  // Special case: the extracted charset is quoted with single quotes.
>+  // For the purpose of preserving what was set we want to handle them
>+  // as delimiters (although they aren't really).
>+  if (charset.Length() >= 2 &&
>+      charset.First() == '\'' &&
>+      charset.Last() == '\'') {
>+    ++mCurPos;
>+    mCurLen -= 2;
>+  }
>+
>+  mCutoff = start;
mCutoff must be still documented. It is really hard to understand without the comment I mentioned in the previous review.

with those, r+

Comment 109

[Thomas Wisniewski]

Attachment: 1285036-10-factor-request-header-code-into-its-own-class.diff

Alright, here's a patch addressing those comments. Carrying over r+.

I'm doing another try run for both patches before requesting check-in, just in case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b142533deeba

Comment 110

[Thomas Wisniewski]

Attachment: 1285036-11-factor-fetch-code-out-into-InitiateFetch.diff

It turned out that there was one bug with the patches, but it just involved nudging an nsCOMPtr declaration up a couple of lines so that it doesn't get freed prematurely.

Since there weren't any substantial changes, and try seems clear this time, I'm carrying over r+. Try results are here, just the usual unrelated intermittents: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c83a5fb27876

Requesting checking of patches 10 and 11.

Comment 111

[Thomas Wisniewski]

Attachment: 1285036-10-factor-request-header-code-into-its-own-class.diff

Actually, I'll upload the patches rebased to inbound, since they might not apply cleanly as-is as they were based to central. Here is 10.

Comment 112

[Thomas Wisniewski]

Attachment: 1285036-11-factor-fetch-code-out-into-InitiateFetch.diff

And here is 11.

Comment 113

[Ryan VanderMeulen [:RyanVM]]

Lots of crashes in that Try push.

Comment 114

[Thomas Wisniewski]

Agreed, for the old patch versions from comment 109. But I addressed those crashes in the latest patches as of comment 110, no? Or am I missing something obvious in that try run in comment 110? It only looks like known (yet unrelated) intermittent failures for that version to me, but I'm still pretty new here...

Comment 115

[Ryan VanderMeulen [:RyanVM]]

Sorry, I missed the link in comment 110.

Comment 116

[Thomas Wisniewski]

Oh no problem, I just wanted to make sure I wasn't missing something important.

I'll bear in mind to re-summarize the situation in my last comment next time, for the sake of expediency and sanity.

Comment 117

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84dcc03312fd
Part 10: Factor the request-header XHR code out into its own class. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce797e5463b
Part 11: Factor the actual fetch-related code out of XHR Send() and into its own function, InitiateFetch(). r=smaug

Comment 118

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

https://hg.mozilla.org/mozilla-central/rev/84dcc03312fd
https://hg.mozilla.org/mozilla-central/rev/5ce797e5463b

Comment 119

[Anne (:annevk)]

Given that bug 1293881 reverted part of the code landed here, what is the status of this bug at this point?

Comment 120

[Thomas Wisniewski]

That revert didn't really affect the refactoring that was done here, it only reverted a change that went a bit too far for a refactor.

Overall I think that the low-hanging fruit is refactored, but there are still refactoring efforts that probably deserve their own bugs now, like merging the worker and main thread implementations, using the fetch driver like the spec does rather than a separate implementation, and possibly converting over to using whatwg streams (once we have a working implementation of those).

Based on that I think we can either close this bug, or maybe convert it into a meta-bug to track other efforts that should definitely be in their own bug (like the ones listed above). Even if there are other smaller refactoring bits that can be done, they might as well go in their own tickets.

Comment 121

[Anne (:annevk)]

Given that we landed a whole bunch of code in the scope of this bug I think I'd prefer closing. It seems bug 726433 is the meta XMLHttpRequest bug.

Comment 122

[Thomas Wisniewski]

Sure, I'll also re-title the bug as well to make it clear it was a partial refactor.