[e10s] RemoteWebNavigation doesn't accept postdata or headers

RESOLVED FIXED in Firefox 42

Status

()

Toolkit
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mkaply, Assigned: Felipe)

Tracking

(Blocks: 1 bug, {addon-compat})

Trunk
mozilla42
x86_64
Windows 7
addon-compat
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(e10sm7+, firefox42 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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.

Updated

3 years ago
tracking-e10s: --- → ?
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.
Flags: needinfo?(mozilla)
(Reporter)

Comment 2

3 years ago
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.
Flags: needinfo?(mozilla)
Thanks. I'll dig in and figure out specifically where the failure is here.
Flags: needinfo?(dtownsend)
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.
Component: Extension Compatibility → General
Flags: needinfo?(dtownsend)
Product: Firefox → Toolkit
Summary: "WEB.DE MailCheck" add-on does not work with e10s - openUILinkIn with POST → [e10s] RemoteWebNavigation doesn't accept postdata or headers
tracking-e10s: ? → +
Duplicate of this bug: 1147135
Duplicate of this bug: 1149911
Looks like this affects opensearch plugins too so may be more important
tracking-e10s: + → ?
tracking-e10s: ? → m7+
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+

Comment 8

2 years ago
Created attachment 8591740 [details] [diff] [review]
Possible patch

I hope I found all the edge cases.
Attachment #8591740 - Flags: review?(dtownsend)
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 on attachment 8591740 [details] [diff] [review]
Possible patch

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

Waiting on answers to those questions.
Attachment #8591740 - Flags: review?(dtownsend)
(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.)

Updated

2 years ago
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
(Assignee)

Comment 12

2 years ago
Created attachment 8604361 [details] [diff] [review]
Test that postdata works with openUILinkUI
Attachment #8604361 - Flags: review?(dtownsend)
(Assignee)

Comment 13

2 years ago
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
Attachment #8591740 - Flags: review?(dtownsend)

Updated

2 years ago
Iteration: 40.3 - 11 May → 41.1 - May 25
Comment on attachment 8591740 [details] [diff] [review]
Possible patch

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

r+ with the nit addressed
Attachment #8591740 - Flags: review?(dtownsend) → review+
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
Attachment #8604361 - Flags: review?(dtownsend) → review+

Comment 16

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

Comment 17

2 years ago
Backout:
https://hg.mozilla.org/integration/fx-team/rev/ef21b61e8820
https://hg.mozilla.org/integration/fx-team/rev/d914fbd54325
Hi, sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3061273&repo=fx-team
Flags: needinfo?(felipc)

Updated

2 years ago
Blocks: 1167915

Comment 19

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/109474240d8c
https://hg.mozilla.org/integration/fx-team/rev/9ac1d4aadc38
(Assignee)

Comment 20

2 years ago
Test failure was caused by typo on original patch, s/postdata/postData/
Flags: needinfo?(felipc)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1167909
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
Flags: needinfo?(felipc)

Comment 23

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

Updated

2 years ago
Duplicate of this bug: 1174027

Comment 25

2 years ago
Any plan for this to re-land?
Both the patch author and reviewer are on PTO until the 21st. Maybe Mike can find someone to fix the failure and re-land?
Flags: needinfo?(mozilla)
(Reporter)

Comment 27

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

Comment 28

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

Comment 29

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8927404a8568
https://hg.mozilla.org/mozilla-central/rev/943a52a8ddbe
https://hg.mozilla.org/mozilla-central/rev/8927404a8568
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Comment 31

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

2 years ago
See bug 1183565.
(Assignee)

Updated

2 years ago
Flags: needinfo?(felipc)
You need to log in before you can comment on or make changes to this bug.