Closed Bug 1269102 Opened 3 years ago Closed 3 years ago

e10s changes header order in copy as curl

Categories

(DevTools :: Netmonitor, defect)

48 Branch
x86_64
Linux
defect
Not set

Tracking

(e10s+, firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
e10s + ---
firefox51 --- fixed

People

(Reporter: colin, Assigned: jsnajdr)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

58 bytes, text/x-review-board-request
ochameau
: review+
Details
58 bytes, text/x-review-board-request
ochameau
: review+
Details
58 bytes, text/x-review-board-request
ochameau
: review+
Details
58 bytes, text/x-review-board-request
ochameau
: review+
Details
58 bytes, text/x-review-board-request
ochameau
: review+
Details
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160429004052

Steps to reproduce:

The order of HTTP headers changes when e10s is enabled.
This test fails on e10s: https://github.com/mozilla/gecko-dev/blob/master/devtools/client/netmonitor/test/browser_net_copy_as_curl.js

When I run the test with ./mach mochitest --disable-e10s it works.

It seems the issue is that the headers aren't returned in the same order.
With e10s disabled, this is the curl string returned:
curl 'http://example.com/browser/devtools/client/netmonitor/test/sjs_simple-test-server.sjs' -H 'Host: example.com' -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0' -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8' -H 'Accept-Language: en-US' --compressed -H 'X-Custom-Header-1: Custom value' -H 'X-Custom-Header-2: 8.8.8.8' -H 'X-Custom-Header-3: Mon, 3 Mar 2014 11:11:11 GMT' -H 'Referer: http://example.com/browser/devtools/client/netmonitor/test/html_copy-as-curl.html' -H 'Connection: keep-alive' -H 'Pragma: no-cache' -H 'Cache-Control: no-cache'

With e10s enabled, this is the curl string returned:
curl 'http://example.com/browser/devtools/client/netmonitor/test/sjs_simple-test-server.sjs' -H 'Host: example.com' -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0' -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8' -H 'Accept-Language: en-US' --compressed -H 'Referer: http://example.com/browser/devtools/client/netmonitor/test/html_copy-as-curl.html' -H 'X-Custom-Header-1: Custom value' -H 'X-Custom-Header-2: 8.8.8.8' -H 'X-Custom-Header-3: Mon, 3 Mar 2014 11:11:11 GMT' -H 'Connection: keep-alive' -H 'Pragma: no-cache' -H 'Cache-Control: no-cache'

The Referer header is added before the custom headers with e10s, and after them with e10s disabled.
Status: UNCONFIRMED → NEW
tracking-e10s: --- → ?
Component: Untriaged → DOM
Ever confirmed: true
OS: Unspecified → Linux
Product: Firefox → Core
Hardware: Unspecified → x86_64
This should probably at least start in devtools (not saying it isn't a platform bug ...).
Component: DOM → Developer Tools: Netmonitor
Product: Core → Firefox
Blocks: dte10s
Has been fixed in bug 1285036 - part 7 patch compares the command line options as sets, ignoring the order. Now I plan to:
- enable the test in e10s again (remove the skip-if)
- refactor the test to use Task.async and fix eslint errors
Assignee: nobody → jsnajdr
Depends on: 1285036
Depends on: 1293881
Depends on: 1273795
This started out as a trivial cleanup and re-enabling of the browser_net_copy_as_curl.js test, and then it snowballed into a five-part saga. The story is:

Part 5: cleanup of browser_net_copy_as_curl.js:
- rewrite to Task.js
- unify EXPECTED_{WIN,POSIX}_RESULT into one constant, using a few utility functions
- fix the comparison algorithm to also check the "--compressed" param (includes a small change in the regexp)
- explain what the regexp does - it's a rather advanced one, so some comment is appropriate
- fix the comparison of the sets (i.e., don't check "size" property on Array)
- use the waitForClipboardPromise utility function

Now, the waitForClipboardPromise function is already copy&pasted at two places (debugger, jsonview) and I didn't want to add a third copy. So, I first moved it up to shared-head.js. That's Part 1 patch.

In Part 2, I include the shared-head.js in client/netmonitor/test/head.js and remove duplicated stuff. There's one trouble with this: the removeTab function in netmonitor/test/head.js uses the skipPermitUnload parameter, while the common definition in shared-head.js doesn't. Removing the parameter breaks a few netmonitor tests.

One of them is browser_net_service-worker-status.js, fixed in bug 1273795.

Second one is browser_net_pane-toggle.js. This test synchronously enables and disables the request details pane and doesn't wait for anything. However, there is an async task triggered by the enable, at [1], which continues to run even after the toolbox is closed and the tab removed. Which means it will fail. If it fails inside the Task.spawn function, the resulting rejected promise is handled by the then() handler and logged with console.error. But if it fails in the then() handler, nobody catches the failure.

It turns out that removing skipPermitUnload changes the timing and the failure is moved from the safe place to the unsafe one.

Part 4 patch fixes this by waiting for the TAB_UPDATE event before proceeding.

Part 3 is just a refactoring that doesn't change any behavior.

Try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65f6fc8d8483&selectedJob=25637875

[1] http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/netmonitor-view.js#3004
Attachment #8780496 - Attachment is obsolete: true
Attachment #8780496 - Flags: review?(poirot.alex)
Attachment #8780497 - Attachment is obsolete: true
Attachment #8780497 - Flags: review?(poirot.alex)
Attachment #8780498 - Attachment is obsolete: true
Attachment #8780498 - Flags: review?(poirot.alex)
Attachment #8780499 - Attachment is obsolete: true
Attachment #8780499 - Flags: review?(poirot.alex)
Rebased and fixed part 5 after bug 1293881 landed - change the case of "x-custom-1" headers to "X-Custom-1".

Learned that "git mozreview push <part-5-commit-id>" cancels review for parts 1-4 instead of just updating the request for part 5. So I had to push once more. Sorry for spamming.
Comment on attachment 8781026 [details]
Bug 1269102 - Part 1: Move waitForClipboardPromise to shared-head.js

https://reviewboard.mozilla.org/r/71522/#review69128

Note that there is another method like this in the inspector land:
http://searchfox.org/mozilla-central/source/devtools/client/inspector/test/head.js#592
I would be happy to see it merged with yours.
Comment on attachment 8781026 [details]
Bug 1269102 - Part 1: Move waitForClipboardPromise to shared-head.js

https://reviewboard.mozilla.org/r/71522/#review69130
Attachment #8781026 - Flags: review?(poirot.alex) → review+
Comment on attachment 8781027 [details]
Bug 1269102 - Part 2: Include shared-head.js in netmonitor/test/head.js, remove duplicate definitions

https://reviewboard.mozilla.org/r/71524/#review69132

Oh! I thought we were already importing shared-head from most test suites...
Thanks for doing that!
Attachment #8781027 - Flags: review?(poirot.alex) → review+
Comment on attachment 8781028 [details]
Bug 1269102 - Part 3: Cleanup browser_net_pane-toggle.js test (task.js, eslint)

https://reviewboard.mozilla.org/r/71526/#review69134

::: devtools/client/netmonitor/test/browser_net_pane-toggle.js:72
(Diff revision 1)
> -        "The details pane should now be hidden after the toggle button was pressed again.");
> +    "The details pane should now be hidden after the toggle button was pressed again.");
> -      is(RequestsMenu.selectedItem, null,
> +  is(RequestsMenu.selectedItem, null,
> -        "There should now be no selected item in the requests menu.");
> +    "There should now be no selected item in the requests menu.");
>  
> -      teardown(aMonitor).then(finish);
> -    });
> +  yield teardown(monitor);
> +  finish();

I'm not sure calling finish is any useful when using add_task
Comment on attachment 8781028 [details]
Bug 1269102 - Part 3: Cleanup browser_net_pane-toggle.js test (task.js, eslint)

https://reviewboard.mozilla.org/r/71526/#review69136
Attachment #8781028 - Flags: review?(poirot.alex) → review+
Comment on attachment 8781029 [details]
Bug 1269102 - Part 4: browser_net_pane-toggle.js: wait for async update after opening the details pane

https://reviewboard.mozilla.org/r/71528/#review69138
Attachment #8781029 - Flags: review?(poirot.alex) → review+
Comment on attachment 8780500 [details]
Bug 1269102 - Part 5: Cleanup browser_net_copy_as_curl.js, reenable for e10s

https://reviewboard.mozilla.org/r/71208/#review69124

::: devtools/client/netmonitor/test/browser.ini:44
(Diff revision 2)
>    sjs_sorting-test-server.sjs
>    sjs_status-codes-test-server.sjs
>    test-image.png
>    service-workers/status-codes.html
>    service-workers/status-codes-service-worker.js
> +  !/devtools/client/framework/test/shared-head.js

Shouldn't this be included in part 2?
Feel free to squash this patches before landing if that helps...

::: devtools/client/netmonitor/test/browser_net_copy_as_curl.js:48
(Diff revision 2)
> -    let { NetMonitorView } = aMonitor.panelWin;
> +  let { NetMonitorView } = monitor.panelWin;
> -    let { RequestsMenu } = NetMonitorView;
> +  let { RequestsMenu } = NetMonitorView;
> -
> -    RequestsMenu.lazyUpdate = false;
> +  RequestsMenu.lazyUpdate = false;
>  
> -    waitForNetworkEvents(aMonitor, 1).then(() => {
> +  debuggee.performRequest(SIMPLE_SJS);

Shouldn't we use a ContentTask here while we are at refactoring this test?

::: devtools/client/netmonitor/test/browser_net_copy_as_curl.js:85
(Diff revision 2)
> -    });
> +  });
>  
> -    aDebuggee.performRequest(SIMPLE_SJS);
> +  info("Clipboard contains a cURL command for the currently selected item's url.");
>  
> -    function cleanUp() {
> -      teardown(aMonitor).then(finish);
> +  yield teardown(monitor);
> +  finish();

Is calling finish any usefull when using add_task?
I think the test framework does that for us:
http://searchfox.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SpawnTask.js#287
Attachment #8780500 - Flags: review?(poirot.alex) → review+
Depends on: 1295556
Submitted new version of the patch with review issues addressed:
- the Inspector waitForClipboardPromise (comment 17) filed as followup bug 1295556.
- don't call finish() in add_task()
- rewrite debuggee CPOW calls to ContentTask
- moved part of the browser.ini patch from part 5 to part 2
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/91197aa84b19
Part 1: Move waitForClipboardPromise to shared-head.js r=ochameau
https://hg.mozilla.org/integration/autoland/rev/9d9607492f45
Part 2: Include shared-head.js in netmonitor/test/head.js, remove duplicate definitions r=ochameau
https://hg.mozilla.org/integration/autoland/rev/09965892a48d
Part 3: Cleanup browser_net_pane-toggle.js test (task.js, eslint) r=ochameau
https://hg.mozilla.org/integration/autoland/rev/6d4061b646e4
Part 4: browser_net_pane-toggle.js: wait for async update after opening the details pane r=ochameau
https://hg.mozilla.org/integration/autoland/rev/123a3bcfafc1
Part 5: Cleanup browser_net_copy_as_curl.js, reenable for e10s r=ochameau
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/c03162882f81
Part 6: Remove the Services import from HAR: Remove Services redeclaration. r=clipbboard-test-fix
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.