Closed
Bug 1129957
Opened 10 years ago
Closed 10 years ago
[e10s] RemoteWebNavigation doesn't accept postdata or headers
Categories
(Toolkit :: General, defect)
Tracking
()
People
(Reporter: mkaply, Assigned: Felipe)
References
Details
(Keywords: addon-compat)
Attachments
(2 files)
8.77 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
2.98 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
tracking-e10s:
--- → ?
Comment 1•10 years ago
|
||
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•10 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)
Comment 3•10 years ago
|
||
Thanks. I'll dig in and figure out specifically where the failure is here.
Flags: needinfo?(dtownsend)
Comment 4•10 years ago
|
||
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
Updated•10 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
Comment 8•10 years ago
|
||
I hope I found all the edge cases.
Attachment #8591740 -
Flags: review?(dtownsend)
Comment 9•10 years ago
|
||
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•10 years ago
|
||
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)
Comment 11•10 years ago
|
||
(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•10 years ago
|
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8604361 -
Flags: review?(dtownsend)
Assignee | ||
Comment 13•10 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•10 years ago
|
Iteration: 40.3 - 11 May → 41.1 - May 25
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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•10 years ago
|
||
Comment 17•10 years ago
|
||
Comment 18•10 years ago
|
||
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)
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
Test failure was caused by typo on original patch, s/postdata/postData/
Flags: needinfo?(felipc)
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•10 years ago
|
||
Comment 25•10 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•10 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•10 years ago
|
||
Comment 29•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/943a52a8ddbe
https://hg.mozilla.org/mozilla-central/rev/8927404a8568
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 31•10 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.
Comment 32•10 years ago
|
||
(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•10 years ago
|
||
See bug 1183565.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(felipc)
You need to log in
before you can comment on or make changes to this bug.
Description
•