1129957 - [e10s] RemoteWebNavigation doesn't accept postdata or headers

Status: RESOLVED FIXED

Description

[Mike Kaply [:mkaply]]

I'm porting the WEB.DE Mailcheck to e10s.

The function openUILinkIn in utilityOverlay.js supports a postData parameter that makes it easy to open a web page with POST data.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#191

This functionality doesn't work on e10s. There are no errors on the console.

I've verified that it works if you turn off e10s on the same build.

Comment 1

[Dave Townsend [:mossop]]

What are the arguments you're passing to openUILinkIn. There may be a couple of bugs here, I know that if a tab is non-remote and you try to load a remote page with post data in it it will fail, because we don't pass the postData to the child process. We may just not pass postData to the child process in all cases though.

Comment 2

[Mike Kaply [:mkaply]]

There's no tab yet:

 openUILinkIn("http://www.foo.com", "tab", false, createPostDataFromString(uploadBody, mimetype));

We're asking openUILinkIn to create the tab and pass the data.

Comment 3

[Dave Townsend [:mossop]]

Thanks. I'll dig in and figure out specifically where the failure is here.

Comment 4

[Dave Townsend [:mossop]]

Sorry it took so long but I found the problem, the call through to tell the child to load doesn't support passing postdata and headers right now. I think that should be straightforward to fix. It will mean that those streams are consumed before the load begins but I doubt any extensions rely on the timing of that.

Comment 7

[Dave Townsend [:mossop]]

Looks like this affects opensearch plugins too so may be more important

Comment 8

[[e-mail]]

Created attachment 8591740 [details] [diff] [review]
Possible patch

I hope I found all the edge cases.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 8591740 [details] [diff] [review]
Possible patch

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

Can we always be sure we can read the post data input stream synchronously? I'm not really sure where post data tends to be created.

Could we add a test for this?

::: toolkit/content/browser-child.js
@@ +21,5 @@
>                                       "nsICrashReporter");
>  }
>  
> +function makeInputStream(aString)
> +{

Nit: brace on preceding line

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8591740 [details] [diff] [review]
Possible patch

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

Waiting on answers to those questions.

Comment 11

[[e-mail]]

(In reply to Dave Townsend from comment #9)
> Can we always be sure we can read the post data input stream synchronously?
> I'm not really sure where post data tends to be created.
Basically it's bookmarks and search engines, and they just create string input streams (or sometimes something slightly more complicated based on string input streams). (I guess file uploads might use async streams, but that's all handled internally.)

> Could we add a test for this?
Feel free to add as many tests as you like.

> > +function makeInputStream(aString)
> > +{
> 
> Nit: brace on preceding line

Even for top-level functions? (This would be the first in this file.)

Comment 12

[:Felipe Gomes (needinfo me!)]

Created attachment 8604361 [details] [diff] [review]
Test that postdata works with openUILinkUI

Comment 13

[:Felipe Gomes (needinfo me!)]

Comment on attachment 8591740 [details] [diff] [review]
Possible patch

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

Re-requesting review on Neil's patch as Neil answered a question in comment 11 and the remaining thing here was to add a test which I did in a separate patch. I'll fix the style nits before landing

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8591740 [details] [diff] [review]
Possible patch

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

r+ with the nit addressed

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 8604361 [details] [diff] [review]
Test that postdata works with openUILinkUI

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

::: browser/base/content/test/general/browser_remoteWebNavigation_postdata.js
@@ +15,5 @@
> +add_task(function* test_remoteWebNavigation_postdata() {
> +  let obj = {};
> +  Cu.import("resource://testing-common/httpd.js", obj);
> +  Cu.import("resource://services-common/utils.js", obj);
> +  

Nit: trailing whitespace

@@ +32,5 @@
> +
> +  let i = server.identity;
> +  let path = i.primaryScheme + "://" + i.primaryHost + ":" + i.primaryPort + "/test";
> +
> +  let postdata = 

Nit: trailing whitespace

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8f9747fc7249
https://hg.mozilla.org/integration/fx-team/rev/375948c55900

Comment 17

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/ef21b61e8820
https://hg.mozilla.org/integration/fx-team/rev/d914fbd54325

Comment 18

[Carsten Book [:Tomcat]]

Hi, sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3061273&repo=fx-team

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/109474240d8c
https://hg.mozilla.org/integration/fx-team/rev/9ac1d4aadc38

Comment 20

[:Felipe Gomes (needinfo me!)]

Test failure was caused by typo on original patch, s/postdata/postData/

Comment 22

[Wes Kocher (:KWierso)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/02bf142b27ad for being the likely cause of frequent timeouts in browser_privatebrowsing_DownloadLastDirWithCPS.js

https://treeherder.mozilla.org/logviewer.html#?job_id=3408008&repo=fx-team

Comment 23

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/1fd19d8fc936

Comment 25

[avada]

Any plan for this to re-land?

Comment 26

[Wes Kocher (:KWierso)]

Both the patch author and reviewer are on PTO until the 21st. Maybe Mike can find someone to fix the failure and re-land?

Comment 27

[Mike Kaply [:mkaply]]

I assume by the typo, they meant:

    browser.webNavigation.loadURIWithOptions(uri, flags, referrer, referrerPolicy,
                                             postdata, null, null);

Can someone remind me how to easily run these tests locally? Will I see the timeouts locally?

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/943a52a8ddbe

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8927404a8568

Comment 30

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/943a52a8ddbe
https://hg.mozilla.org/mozilla-central/rev/8927404a8568

Comment 31

[blackwind]

Instead of the original error message, I'm now getting this in Nightly when trying to pass postdata:

> JavaScript error: resource://gre/modules/NetUtil.jsm, line 456: NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIScriptableInputStream.init]

The code continues to work as expected in non-e10s windows.

Comment 32

[Dave Townsend [:mossop]]

(In reply to blackwind from comment #31)
> Instead of the original error message, I'm now getting this in Nightly when
> trying to pass postdata:
> 
> > JavaScript error: resource://gre/modules/NetUtil.jsm, line 456: NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIScriptableInputStream.init]
> 
> The code continues to work as expected in non-e10s windows.

Please file a new bug with example code that reproduces this.

Comment 33

[blackwind]

See bug 1183565.