Closed Bug 1129957 Opened 10 years ago Closed 10 years ago

[e10s] RemoteWebNavigation doesn't accept postdata or headers

Categories

(Toolkit :: General, defect)

x86_64
Windows 7
defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
mozilla42
Iteration:
41.1 - May 25
Tracking Status
e10s m7+ ---
firefox42 --- fixed

People

(Reporter: mkaply, Assigned: Felipe)

References

Details

(Keywords: addon-compat)

Attachments

(2 files)

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.
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)
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
Looks like this affects opensearch plugins too so may be more important
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
Attached patch Possible patchSplinter Review
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.)
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
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)
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+
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)
Blocks: 1167915
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)
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)
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)
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.
Flags: needinfo?(felipc)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: