Refactor some of the code in XMLHttpRequest

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Thomas Wisniewski, Assigned: Thomas Wisniewski, Mentored)

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 40 obsolete attachments)

14.37 KB, patch
Details | Diff | Splinter Review
46.09 KB, patch
Details | Diff | Splinter Review
8.30 KB, patch
Details | Diff | Splinter Review
15.62 KB, patch
Details | Diff | Splinter Review
3.32 KB, patch
Details | Diff | Splinter Review
5.79 KB, patch
Details | Diff | Splinter Review
18.94 KB, patch
Details | Diff | Splinter Review
24.76 KB, patch
Details | Diff | Splinter Review
26.63 KB, patch
Details | Diff | Splinter Review
18.90 KB, patch
Details | Diff | Splinter Review
24.36 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8768570 [details] [diff] [review]
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.
Assignee: nobody → wisniewskit
Attachment #8768570 - Flags: review?(amarchesini)
(Assignee)

Comment 2

2 years ago
Created attachment 8768572 [details] [diff] [review]
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).
Attachment #8768572 - Flags: review?(amarchesini)
(Assignee)

Comment 3

2 years ago
Created attachment 8768573 [details] [diff] [review]
1285036-2-flip_the_async_flag_to_match_spec.diff

3 - Invert mFlagAsynchronous as mFlagSynchronous, to match the spec's wording.
Attachment #8768573 - Flags: review?(amarchesini)
(Assignee)

Comment 4

2 years ago
Created attachment 8768574 [details] [diff] [review]
1285036-4-remove-sent-pseudostate.diff

4 - Remove the "sent" pseudo-state and just add support for the spec's send() flag instead.
Attachment #8768574 - Flags: review?(amarchesini)
(Assignee)

Comment 5

2 years ago
Created attachment 8768575 [details] [diff] [review]
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.
Attachment #8768575 - Flags: review?(amarchesini)
(Assignee)

Comment 6

2 years ago
Created attachment 8768576 [details] [diff] [review]
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.
Attachment #8768576 - Flags: review?(amarchesini)
(Assignee)

Comment 7

2 years ago
Created attachment 8768577 [details] [diff] [review]
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.
Attachment #8768577 - Flags: review?(amarchesini)
(Assignee)

Comment 8

2 years ago
Created attachment 8768578 [details] [diff] [review]
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.
Attachment #8768578 - Flags: review?(amarchesini)
(Assignee)

Comment 9

2 years ago
Created attachment 8768582 [details] [diff] [review]
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.
Attachment #8768582 - Flags: review?(amarchesini)
(Assignee)

Comment 10

2 years ago
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 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.
Attachment #8768570 - Flags: review?(amarchesini) → review+
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?
Attachment #8768572 - Flags: review?(amarchesini) → review+
Attachment #8768573 - Flags: review?(amarchesini) → review+
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.
Attachment #8768574 - Flags: review?(amarchesini) → review+
Attachment #8768575 - Flags: review?(amarchesini) → review+
Attachment #8768576 - Flags: review?(amarchesini) → review+
Comment on attachment 8768577 [details] [diff] [review]
1285036-7-refactor_SetRequestHeader.diff

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

This is patch 6.
Attachment #8768577 - Flags: review?(amarchesini)
(Assignee)

Comment 15

2 years ago
Created attachment 8768804 [details] [diff] [review]
1285036-1-enum-and-constify-event-strings.diff

Added static assertion. Carrying over r=baku.
Attachment #8768570 - Attachment is obsolete: true
(Assignee)

Comment 16

2 years ago
Created attachment 8768818 [details] [diff] [review]
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.
Attachment #8768572 - Attachment is obsolete: true
(Assignee)

Comment 17

2 years ago
Created attachment 8768820 [details] [diff] [review]
1285036-3-flip_the_async_flag_to_match_spec.diff

Re-attaching rebased version with correct filename (part 3, not 2). Carrying over r+.
Attachment #8768573 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
Created attachment 8768829 [details] [diff] [review]
1285036-4-remove-sent-pseudostate.diff

Review comments addressed. Carrying over r+.
Attachment #8768574 - Attachment is obsolete: true
(Assignee)

Comment 19

2 years ago
Created attachment 8768831 [details] [diff] [review]
1285036-4-remove-sent-pseudostate.diff

Actually carry over the r+ in the checkin comment this time...
Attachment #8768829 - Attachment is obsolete: true
(Assignee)

Comment 20

2 years ago
Created attachment 8768833 [details] [diff] [review]
1285036-5-consolidate_FireReadystatechange_code.diff

Might as well rebase this while I'm here. Carrying over r+.
Attachment #8768575 - Attachment is obsolete: true
(Assignee)

Comment 21

2 years ago
Created attachment 8768835 [details] [diff] [review]
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?
Attachment #8768577 - Attachment is obsolete: true
Attachment #8768835 - Flags: review?(amarchesini)
Sure, just add the keyword "leave-open".
(Assignee)

Comment 23

2 years ago
Alright, thanks.

I'm requesting checkin for the reviewed patches, 1 to 6.
Keywords: checkin-needed, leave-open

Comment 24

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
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

2 years ago
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f5af40eb365
Backed out 6 changesets for bustage on a CLOSED TREE.
(Assignee)

Comment 27

2 years ago
Created attachment 8769269 [details] [diff] [review]
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!
Attachment #8768804 - Attachment is obsolete: true
(Assignee)

Comment 28

2 years ago
Created attachment 8769270 [details] [diff] [review]
1285036-2-use_an_enum_for_state_and_bools_for_flags.diff
Attachment #8768818 - Attachment is obsolete: true
(Assignee)

Comment 29

2 years ago
Created attachment 8769281 [details] [diff] [review]
1285036-3-flip_the_async_flag_to_match_spec.diff
Attachment #8768820 - Attachment is obsolete: true
(Assignee)

Comment 30

2 years ago
Created attachment 8769282 [details] [diff] [review]
1285036-4-remove-sent-pseudostate.diff
Attachment #8768831 - Attachment is obsolete: true
(Assignee)

Comment 31

2 years ago
Created attachment 8769283 [details] [diff] [review]
1285036-5-consolidate_FireReadystatechange_code.diff
Attachment #8768833 - Attachment is obsolete: true
(Assignee)

Comment 32

2 years ago
Created attachment 8769284 [details] [diff] [review]
1285036-6-add_CString_ASCIIToLower_Upper.diff
Attachment #8768576 - Attachment is obsolete: true
(Assignee)

Comment 33

2 years ago
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.
Keywords: checkin-needed
(Assignee)

Comment 34

2 years ago
Created attachment 8769528 [details] [diff] [review]
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.
Attachment #8769528 - Flags: review?(amarchesini)
(Assignee)

Comment 35

2 years ago
Created attachment 8769529 [details] [diff] [review]
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.
Attachment #8769529 - Flags: review?(amarchesini)
(Assignee)

Comment 36

2 years ago
Created attachment 8769548 [details] [diff] [review]
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.
Attachment #8768578 - Attachment is obsolete: true
Attachment #8768578 - Flags: review?(amarchesini)
Attachment #8769548 - Flags: review?(amarchesini)

Comment 37

2 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
(Assignee)

Comment 39

2 years ago
Created attachment 8771700 [details] [diff] [review]
1285036-7-refactor_SetRequestHeader.diff

Rebased patch 7.
Attachment #8768835 - Attachment is obsolete: true
Attachment #8768835 - Flags: review?(amarchesini)
Attachment #8771700 - Flags: review?(amarchesini)
(Assignee)

Comment 40

2 years ago
Created attachment 8771713 [details] [diff] [review]
1285036-9-refactor_Send_endpoints_and_RequestBody.diff

Rebased patch 9.
Attachment #8768582 - Attachment is obsolete: true
Attachment #8768582 - Flags: review?(amarchesini)
Attachment #8771713 - Flags: review?(amarchesini)
(Assignee)

Comment 41

2 years ago
Created attachment 8771714 [details] [diff] [review]
1285036-10-refactor_request_header_handling.diff

Rebased patch 10.
Attachment #8769528 - Attachment is obsolete: true
Attachment #8769528 - Flags: review?(amarchesini)
Attachment #8771714 - Flags: review?(amarchesini)
(Assignee)

Comment 42

2 years ago
Created attachment 8771715 [details] [diff] [review]
1285036-11-factor_channel_setup_code_out_of_send.diff

Rebased patch 11.
Attachment #8769529 - Attachment is obsolete: true
Attachment #8769529 - Flags: review?(amarchesini)
Attachment #8771715 - Flags: review?(amarchesini)
(Assignee)

Comment 43

2 years ago
A fresh try-run for rebased patches 7-11 seems ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b0be446311a
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
Attachment #8771700 - Flags: review?(amarchesini) → review+
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
Attachment #8769548 - Flags: review?(amarchesini) → review+
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
Attachment #8771713 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 47

2 years ago
> 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.
(Assignee)

Comment 48

2 years ago
Created attachment 8772254 [details] [diff] [review]
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
Attachment #8771700 - Attachment is obsolete: true
(Assignee)

Comment 49

2 years ago
Created attachment 8772255 [details] [diff] [review]
1285036-8-refactor_Open.diff
Attachment #8769548 - Attachment is obsolete: true
(Assignee)

Comment 50

2 years ago
Created attachment 8772256 [details] [diff] [review]
1285036-9-refactor_Send_endpoints_and_RequestBody.diff
Attachment #8771713 - Attachment is obsolete: true
(Assignee)

Comment 51

2 years ago
Requesting checkin of patches 7, 8 and 9.
Keywords: checkin-needed
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
Flags: needinfo?(wisniewskit)
Keywords: checkin-needed
(Assignee)

Comment 53

2 years ago
My bad, I wasn't paying enough attention to whether my checkin-requests would clash. I'll slow it down.
Flags: needinfo?(wisniewskit)
(Assignee)

Comment 54

2 years ago
Created attachment 8772979 [details] [diff] [review]
1285036-7-refactor_SetRequestHeader.diff

Rebased patches incoming, carrying over r+ for 7-9.
Attachment #8772254 - Attachment is obsolete: true
(Assignee)

Comment 55

2 years ago
Created attachment 8772980 [details] [diff] [review]
1285036-8-refactor_Open.diff
Attachment #8772255 - Attachment is obsolete: true
(Assignee)

Comment 56

2 years ago
Created attachment 8772982 [details] [diff] [review]
1285036-9-refactor_Send_endpoints_and_RequestBody.diff
Attachment #8772256 - Attachment is obsolete: true
(Assignee)

Comment 57

2 years ago
Created attachment 8772983 [details] [diff] [review]
1285036-10-refactor_request_header_handling.diff
Attachment #8771714 - Attachment is obsolete: true
Attachment #8771714 - Flags: review?(amarchesini)
Attachment #8772983 - Flags: review?(amarchesini)
(Assignee)

Comment 58

2 years ago
Created attachment 8772986 [details] [diff] [review]
1285036-11-factor_channel_setup_code_out_of_send.diff
Attachment #8771715 - Attachment is obsolete: true
Attachment #8771715 - Flags: review?(amarchesini)
Attachment #8772986 - Flags: review?(amarchesini)
(Assignee)

Comment 59

2 years ago
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.
Keywords: checkin-needed

Comment 60

2 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 62

2 years ago
Backout by cbook@mozilla.com:
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
(Assignee)

Comment 63

2 years ago
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!
Flags: needinfo?(wisniewskit)
(Assignee)

Comment 64

2 years ago
Created attachment 8773301 [details] [diff] [review]
1285036-9-refactor_Send_endpoints_and_RequestBody.diff

Patch 9, now with more explicit. Carrying over r+.
Attachment #8772982 - Attachment is obsolete: true
(Assignee)

Comment 65

2 years ago
Created attachment 8773302 [details] [diff] [review]
1285036-10-refactor_request_header_handling.diff

Patch 10, now with more explicit. Carrying over r+.
Attachment #8772983 - Attachment is obsolete: true
Attachment #8772983 - Flags: review?(amarchesini)
(Assignee)

Updated

2 years ago
Attachment #8773302 - Flags: review?(amarchesini)
(Assignee)

Comment 66

2 years ago
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
Keywords: checkin-needed

Comment 67

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Thomas, is there a chance that this work would fix bug 827243?
Flags: needinfo?(wisniewskit)
(Assignee)

Comment 71

2 years ago
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.
Flags: needinfo?(wisniewskit)
(Assignee)

Comment 72

2 years ago
Created attachment 8775421 [details] [diff] [review]
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.
Attachment #8772979 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 73

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
(Assignee)

Comment 75

2 years ago
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!
Flags: needinfo?(wisniewskit)
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
Attachment #8772986 - Flags: review?(amarchesini) → review-
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?
Attachment #8773302 - Flags: review?(amarchesini) → review?(bugs)
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.
Attachment #8773302 - Flags: review?(bugs) → review-
(Assignee)

Comment 79

2 years ago
Created attachment 8777178 [details] [diff] [review]
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
Attachment #8775421 - Attachment is obsolete: true
(Assignee)

Comment 80

2 years ago
Created attachment 8777179 [details] [diff] [review]
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.
Attachment #8773301 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 81

2 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Duplicate of this bug: 918763
(Assignee)

Comment 84

2 years ago
Created attachment 8779054 [details] [diff] [review]
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.
Attachment #8773302 - Attachment is obsolete: true
Attachment #8779054 - Flags: review?(bugs)
(Assignee)

Comment 85

2 years ago
Created attachment 8779055 [details] [diff] [review]
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).
Attachment #8772986 - Attachment is obsolete: true
Attachment #8779055 - Flags: review?(bugs)
(Assignee)

Comment 86

2 years ago
Oh, wait... you actually *were* reviewing patch 10, baku had just skipped to 11. My bad, sorry for any undue confusion.
Blocks: 1269102
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
Flags: needinfo?(wisniewskit)
(Assignee)

Comment 88

2 years ago
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!
Flags: needinfo?(wisniewskit)
(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.
(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 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.
Attachment #8779054 - Flags: review?(bugs) → review-
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.
Attachment #8779055 - Flags: review?(bugs) → review-
(Assignee)

Comment 93

2 years ago
>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...
(Assignee)

Updated

2 years ago
Depends on: 1293881
(Assignee)

Comment 94

2 years ago
Created attachment 8779602 [details] [diff] [review]
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.)
Attachment #8779054 - Attachment is obsolete: true
Attachment #8779602 - Flags: review?(bugs)
(Assignee)

Comment 95

2 years ago
Created attachment 8779796 [details] [diff] [review]
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).
Attachment #8779055 - Attachment is obsolete: true
Attachment #8779796 - Flags: review?(bugs)
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 <wisniewskit@gmail.com>
># 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
Attachment #8779602 - Flags: review?(bugs) → review-
(Assignee)

Comment 97

2 years ago
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.
(Assignee)

Comment 98

2 years ago
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.
ok, thanks for testing. If necko anyhow orders the headers somehow, then fine.

Updated

2 years ago
Attachment #8779602 - Flags: review- → review?(bugs)
But, any reason why you order header setting differently to what the spec does?
(Assignee)

Comment 101

2 years ago
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).
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.
(Assignee)

Comment 103

2 years ago
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 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
Attachment #8779602 - Flags: review?(bugs) → review-
That accept handling can be as it is in the patch.
Comment on attachment 8779796 [details] [diff] [review]
1285036-11-factor-fetch-code-out-into-InitiateFetch.diff

ok, mostly code move.
Attachment #8779796 - Flags: review?(bugs) → review+
(Assignee)

Comment 107

2 years ago
Created attachment 8779924 [details] [diff] [review]
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.
Attachment #8779602 - Attachment is obsolete: true
Attachment #8779924 - Flags: review?(bugs)
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+
Attachment #8779924 - Flags: review?(bugs) → review+
(Assignee)

Comment 109

2 years ago
Created attachment 8779933 [details] [diff] [review]
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
Attachment #8779924 - Attachment is obsolete: true
(Assignee)

Comment 110

a year ago
Created attachment 8782700 [details] [diff] [review]
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.
Attachment #8779796 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 111

a year ago
Created attachment 8782702 [details] [diff] [review]
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.
Attachment #8779933 - Attachment is obsolete: true
(Assignee)

Comment 112

a year ago
Created attachment 8782703 [details] [diff] [review]
1285036-11-factor-fetch-code-out-into-InitiateFetch.diff

And here is 11.
Attachment #8782700 - Attachment is obsolete: true
Lots of crashes in that Try push.
Keywords: checkin-needed
(Assignee)

Comment 114

a year ago
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...
Flags: needinfo?(ryanvm)
Sorry, I missed the link in comment 110.
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
(Assignee)

Comment 116

a year ago
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

a year ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 119

a year ago
Given that bug 1293881 reverted part of the code landed here, what is the status of this bug at this point?
(Assignee)

Comment 120

a year ago
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

a year ago
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.
(Assignee)

Comment 122

a year ago
Sure, I'll also re-title the bug as well to make it clear it was a partial refactor.
Status: NEW → RESOLVED
Last Resolved: a year ago
Keywords: leave-open
Resolution: --- → FIXED
Summary: Refactor the code for XMLHttpRequest → Refactor some of the code in XMLHttpRequest
Depends on: 1334877
You need to log in before you can comment on or make changes to this bug.