1328761 - Forever [Uploading...] in Gecko Profiler 2.0.13

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Alice0775 White]

Steps To Reproduce:
1. Ctrl+Shift+2
2. Wait to finish retrieve symbol etc.
3. Click [Shear...] button
4. Click [Shear] button

Actual Results:
Forever [Uploading...] 

Expected Results:
URL link should be shown (I do not know exact behavior ov ver2.0.13)

Comment 1

[Alice0775 White]

typo
s/Shear/Share/g

Comment 2

[Markus Stange [:mstange]]

Thanks Alice. I've moved this to https://github.com/mstange/cleopatra/issues/108 .
I haven't seen this myself; I suspect I'll need to add some debug messages that print to the console if things go wrong.
Do you see the request to profile-store.appspot.com in the devtools network panel if you click the Share button?

Comment 3

[Alice0775 White]

Attachment: screenshot

Screenshot(after step 4 and waited >1min)

it requested to profile-store.appspot.com, but response is 0 byte....

Comment 4

[Markus Stange [:mstange]]

Interesting! What are the response headers? Are there any warnings in the console?

Comment 5

[Alice0775 White]

Web console:

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://profile-store.appspot.com/compressed-store. (Reason: missing token ‘content-type’ in CORS header ‘Access-Control-Allow-Headers’ from CORS preflight channel).

uncaught exception: 0

Comment 6

[Markus Stange [:mstange]]

Thanks, that explains why it fails... somewhat. On my machine, the response headers of the OPTIONS request start with
Access-Control-Allow-Origin: *
Do you get something else?

Comment 7

[Markus Stange [:mstange]]

Attachment: screenshot of my network panel

Comment 8

[Alice0775 White]

Attachment: the response headers

Comment 9

[Markus Stange [:mstange]]

Ok, so your request headers contain a "Access-Control-Request-Headers: content-type" header. My request headers do not contain this header. I wonder why.

The server would need to reply with a "Access-Control-Allow-Headers" response header that contains "content-type", e.g.:
"Access-Control-Allow-Headers: Content-Type, Access-Control-Allow-Headers"
But apparently it's not configured to do that.

Bz, do you know why Alice's and my request headers might differ? The code for the XHR is at https://github.com/mstange/cleopatra/blob/cleopatra-react/src/content/cleopatra-profile-store.js .

Comment 10

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #9)
> Ok, so your request headers contain a "Access-Control-Request-Headers:
> content-type" header. My request headers do not contain this header. I
> wonder why.

> Bz, do you know why Alice's and my request headers might differ? The code
> for the XHR is at
> https://github.com/mstange/cleopatra/blob/cleopatra-react/src/content/cleopatra-profile-store.js .

Comment 11

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #9)
> The server would need to reply with a "Access-Control-Allow-Headers"
> response header that contains "content-type", e.g.:
> "Access-Control-Allow-Headers: Content-Type, Access-Control-Allow-Headers"
> But apparently it's not configured to do that.

Jeff, can you configure the server to send those headers?

Comment 12

[Markus Stange [:mstange]]

Alice, what Firefox build are you using? I'm using yesterday's nightly, Built from https://hg.mozilla.org/mozilla-central/rev/f13abb8ba9f366c9f32a3146245adf642528becd 53.0a1 20170105030229

Comment 13

[Alice0775 White]

(In reply to Markus Stange [:mstange] from comment #12)
> Alice, what Firefox build are you using? I'm using yesterday's nightly,
> Built from
> https://hg.mozilla.org/mozilla-central/rev/
> f13abb8ba9f366c9f32a3146245adf642528becd 53.0a1 20170105030229

[App]
Vendor=Mozilla
Name=Firefox
RemotingName=firefox
CodeName=Nightly
Version=53.0a1
BuildID=20170105030229
SourceRepository=https://hg.mozilla.org/mozilla-central
SourceStamp=f13abb8ba9f366c9f32a3146245adf642528becd
ID={ec8030f7-c20a-464f-9b0e-13a3a9e97384}

Comment 14

[Alice0775 White]

I found that disabling e10s seems to cause this problem.

If enable e10s, the problem seems to fix.

Comment 15

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e20fe9571c84457ff1e32fb2183edb8bf8054d7f&tochange=01c1359222ea6f86699b771d306c06137dcf78aa

Regressed by: Bug 1207233

Comment 16

[Boris Zbarsky [:bzbarsky]]

Hmm.  That's really odd. The behavior you describe happened with the _first_ attempt to patch bug 1207233, and broke Cleopatra, but the thing that landed in the regression window you note should have fixed that problem.

And we have a test that tests that we _don't_ set a Content-Type header in this case, and we're passing it.  See http://searchfox.org/mozilla-central/source/testing/web-platform/tests/XMLHttpRequest/setrequestheader-content-type.htm the "ArrayBufferView request sends no Content-Type without setRequestHeader() call" bit.  That's Cleopatra's situation.

I also just checked on http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm in both e10s and non-e10s mode, and we pass the ArrayBufferView test both ways.

Alice0775, what happens when you run that test in your browser in non-e10s mode?  Do you have any extensions that might be relevant?

Comment 17

[Alice0775 White]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #16)

> Alice0775, what happens when you run that test in your browser in non-e10s
> mode?

Tested on Nightly53.0a1(2016-01-05):

#1 with newly created profile(e10s is enabled by default):

Summary
Harness status: OK
Found 32 tests
29 Pass
3 Fail

Details
*snip passed results*

Fail	ReadableStream request respects setRequestHeader("")	assert_unreached: Skipping test as could not create a ReadableStream; Reached unreachable code

request/<@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:17:13
Test.prototype.step@http://w3c-test.org/resources/testharness.js:1406:20
test@http://w3c-test.org/resources/testharness.js:501:9
request@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:13:9
@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:217:7

Fail	ReadableStream request with under type sends no Content-Type without setRequestHeader() call	assert_unreached: Skipping test as could not create a ReadableStream; Reached unreachable code

request/<@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:17:13
Test.prototype.step@http://w3c-test.org/resources/testharness.js:1406:20
test@http://w3c-test.org/resources/testharness.js:501:9
request@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:13:9
@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:223:7

Fail	ReadableStream request keeps setRequestHeader() Content-Type and charset	assert_unreached: Skipping test as could not create a ReadableStream; Reached unreachable code

request/<@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:17:13
Test.prototype.step@http://w3c-test.org/resources/testharness.js:1406:20
test@http://w3c-test.org/resources/testharness.js:501:9
request@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:13:9
@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:229:7

=======================================================================

#2 with with newly created profile, and disable e10s from options:

Summary
Harness status: OK
Found 32 tests
29 Pass
3 Fail

Details
*snip passed results*

Fail	ReadableStream request respects setRequestHeader("")	assert_unreached: Skipping test as could not create a ReadableStream; Reached unreachable code

request/<@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:17:13
Test.prototype.step@http://w3c-test.org/resources/testharness.js:1406:20
test@http://w3c-test.org/resources/testharness.js:501:9
request@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:13:9
@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:217:7

Fail	ReadableStream request with under type sends no Content-Type without setRequestHeader() call	assert_unreached: Skipping test as could not create a ReadableStream; Reached unreachable code

request/<@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:17:13
Test.prototype.step@http://w3c-test.org/resources/testharness.js:1406:20
test@http://w3c-test.org/resources/testharness.js:501:9
request@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:13:9
@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:223:7

Fail	ReadableStream request keeps setRequestHeader() Content-Type and charset	assert_unreached: Skipping test as could not create a ReadableStream; Reached unreachable code

request/<@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:17:13
Test.prototype.step@http://w3c-test.org/resources/testharness.js:1406:20
test@http://w3c-test.org/resources/testharness.js:501:9
request@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:13:9
@http://w3c-test.org/XMLHttpRequest/setrequestheader-content-type.htm:229:7


==================================================================

> Do you have any extensions that might be relevant?

Gecko Profiler only.
No the other addon is installed.

Comment 18

[Alice0775 White]

#3 with newly created profile + Gecko Profiler(e10s is enabled by default):
#4 with newly created profile + Gecko Profiler and disable e10s from options:

I got same results of Comment#17.

Comment 19

[Alice0775 White]

I can also reproduce the problem on Ubuntu16.04LTS and Windows7SP1 when e10s is disabled

Comment 20

[Boris Zbarsky [:bzbarsky]]

Right, Gecko doesn't implement ReadableStream.  But the point is what you're seeing is that the relevant test is passing in all your configurations.

So I stepped through this, and the issue comes from the change to HttpBaseChannel::ExplicitSetUploadStream.  The old code was:

    SetRequestHeader(NS_LITERAL_CSTRING("Content-Type"), aContentType,
                     false);

The new code is:

    if (!aContentType.IsVoid()) {
      if (aContentType.IsEmpty()) {
        SetEmptyRequestHeader(NS_LITERAL_CSTRING("Content-Type"));
      } else {
        SetRequestHeader(NS_LITERAL_CSTRING("Content-Type"), aContentType,
                         false);
      }
    }

Now the thing is, SetRequestHeader() with an empty header value will _remove_ the header when the third arg is false.  So when !aContentType.IsVoid() && aContentType.IsEmpty() we changed behavior: we used to not send a Content-Type header and now we send one with value "".

The same changeset modified XHR to do the right thing and pass a void string to ExplicitSetUploadStream.

But in this testcase, we're calling ExplicitSetUploadStream from HttpBaseChannel::SetupReplacementChannel.  That's happening because we're ending up in InterceptedChannelChrome::ResetInterception which presumably has to do with service worker usage on the page or something?

Anyway, there aren't that many callers of ExplicitSetUploadStream, thankfully.  The one in Navigator::SendBeacon looks buggy in the same way to me.  The one in FetchDriver::Fetch, I can't tell.  The on in SetupReplacementChannel is buggy.  The one in XHR is OK.  The other ones all pass in nonempty strings for the type anyway, afaict.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Ben, can you check whether the FetchDriver behavior is correct?  Can mRequest->Headers() not have a content-type header set?  Comments make it sound like no, but per spec at step 35 of the Request ctor, sure sounds like it could if there is no body or in various other cases in which https://fetch.spec.whatwg.org/#concept-bodyinit-extract returns null...  I guess the comments are a bit misleading since result.Failed() can only happen on invalid header name, and we control the name, and this has nothing to do with the _setting_ of content-type?

And if there is no header our string will end up void, looks like.  So this one looks ok.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I'll leave the beacon thing for bug 1329298.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Argh, and actual needinfo for Ben for comment 21.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Correctly propagate the Content-Type request header during redirects, for cases when it's _not_ set

This fixes the steps from comment 0.  I could really use help with writing a testcase for this...

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Correctly propagate the Content-Type request header during redirects, for cases when it's _not_ set

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

I agree that the content-type header can be void in FetchDriver.  It seems this all works correctly, but the comment is wrong.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Update some FetchDriver comments to more clearly reflect reality

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8824557 [details] [diff] [review]
part 2.  Update some FetchDriver comments to more clearly reflect reality

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

Thanks!

Comment 29

[Dragana Damjanovic [:dragana]]

Comment on attachment 8824529 [details] [diff] [review]
Correctly propagate the Content-Type request header during redirects, for cases when it's _not_ set

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

Sorry for the delay

Comment 30

[Boris Zbarsky [:bzbarsky]]

Try caught a bug: "if (contentType.IsEmpty())" should be "if (contentTypeArg.IsEmpty())" in HttpBaseChannel::SetUploadStream.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Hmm.  Markus, did you change the server side here by any chance?  I just tried to reproduce again (so I could test that the patch with the fix from comment 30 still fixes the original bug), but now I can't reproduce at all, even with a vanilla build without this patch...

Comment 32

[Alice0775 White]

I can reproduce the problem if e10s is disabled.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Oh, right, I forgot that e10s disabled is needed!  OK, good, the patch still fixes things.

I'll land this now, then think about a test for this on Tuesday...

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/750307a1e3f9
part 1.  Correctly propagate the Content-Type request header during redirects, for cases when it's _not_ set.  r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/18ab7878c67b
part 2.  Update some FetchDriver comments to more clearly reflect reality.  r=bkelly

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8824529 [details] [diff] [review]
Correctly propagate the Content-Type request header during redirects, for cases when it's _not_ set

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1207233 
[User impact if declined]: Some sites may be broken.  Basically anything
   involving cross-site XHR and a redirect, and maybe anything that involves
   cross-site XHR and serviceworkers.
[Is this code covered by automated tests?]:  Clearly not enough.  I'll add more
   before landing this on Auroda.
[Has the fix been verified in Nightly?]:  Not yet, but manually verified in
   my build.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Somewhat.  That, and the fact that we apparently
   already shipped this bug in Firefox 50, is why I'm only requesting Aurora
   uplift, not beta.  But I think we expect service workers to be more and 
   more popular, so it would be good to have this bug in the wild as little
   as we have risk tolerance for.
[Why is the change risky/not risky?]: It's changing some pretty core HTTP code.
   I think it's changing it correctly, but clearly that's what we thought in
   the bug that caused this one.
[String changes made/needed]: None

Comment 36

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/750307a1e3f9
https://hg.mozilla.org/mozilla-central/rev/18ab7878c67b

Comment 37

[Julien Cristau [:jcristau]]

Comment on attachment 8824529 [details] [diff] [review]
Correctly propagate the Content-Type request header during redirects, for cases when it's _not_ set

content-type handling fix, aurora52+, hopefully with more tests coming as mentioned in comment 35

Comment 38

[Boris Zbarsky [:bzbarsky]]

Attachment: Tests

I checked that we fail the new test with the typed array without this patch, and pass it with the patch

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eeaa7ae6b3a
Tests. r=bkelly

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e9635dd5012
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d2e70db03c3
https://hg.mozilla.org/releases/mozilla-aurora/rev/29d706c05b1d

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1eeaa7ae6b3a