Closed
Bug 1351702
Opened 8 years ago
Closed 8 years ago
tab-content.js doesn't implement nsIWebBrowserChrome3::shouldLoadURI correctly
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox53 | + | verified |
firefox54 | --- | verified |
firefox55 | --- | verified |
People
(Reporter: bobowen, Assigned: bobowen)
References
Details
(Keywords: regression)
Attachments
(2 files)
(deleted),
patch
|
nika
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
text/html
|
Details |
The patch in bug 1347983 didn't change shouldLoadURI in tab-content.js.
So, aTriggeringPrincipal is set to a bool and even if we try and redirect the load it fails trying to serialise the principal.
[Tracking Requested - why for this release]:
Bug 1347983 has been uplifted to Fx53.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8852549 -
Flags: review?(michael)
Comment 2•8 years ago
|
||
Comment on attachment 8852549 [details] [diff] [review]
Correct shouldLoadURI in tab-content.js to add aHasPostData argument
Review of attachment 8852549 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for catching this!
Attachment #8852549 -
Flags: review?(michael) → review+
Assignee | ||
Comment 3•8 years ago
|
||
Looks like pulsebot is out of action:
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/09c3073768242d0917af5b633cf0c94fd37571d3
Comment 4•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8852549 [details] [diff] [review]
Correct shouldLoadURI in tab-content.js to add aHasPostData argument
Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1347983 - which has been uplifted to Fx53.
[User impact if declined]:
I don't have an actual test case showing an issue (in Fx53) that this causes, but as it stands the call to shouldLoadURI from nsDocShell when doing a POST would always fail, even if we are in a situation where the load should occur in another process.
For non-POST calls the security principal will not be serialised.
This definitely affects the file content process in Fx55.
[Is this code covered by automated tests?]:
The code is exercised by tests, but clearly not this particular scenario.
[Has the fix been verified in Nightly?]:
Problem was found while working on bug 1351358 and fix verified on local build.
[Needs manual test from QE? If yes, steps to reproduce]:
Yes, I'll upload a test case for the file content process with STR which can be used in Nightly.
[List of other uplifts needed for the feature/fix]:
None.
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
Very small change, which corrects a definite error in the code.
[String changes made/needed]:
No.
Attachment #8852549 -
Flags: approval-mozilla-beta?
Attachment #8852549 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 6•8 years ago
|
||
testcase |
This can be used as a test case for Nightly.
* Save file-post-test.html locally.
* Ctrl-O and open file as file: URI.
* Click button.
In Nightly before this fix landed the post data is posted, but only because the process switch does not occur and you can see the following error in the browser console:
NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsISerializationHelper.serializeToString]
After this fix the process switches and the data is not posted, this is an exiting bug and will be address in bug 1351358.
I am a little worried we are still cleaning up issues from bug 1331083.
Keywords: regression
Comment on attachment 8852549 [details] [diff] [review]
Correct shouldLoadURI in tab-content.js to add aHasPostData argument
Fix for a regression in 53, let's uplift to beta.
Attachment #8852549 -
Flags: approval-mozilla-beta?
Attachment #8852549 -
Flags: approval-mozilla-beta+
Attachment #8852549 -
Flags: approval-mozilla-aurora?
Attachment #8852549 -
Flags: approval-mozilla-aurora+
Comment 9•8 years ago
|
||
bugherder uplift |
Comment 10•8 years ago
|
||
bugherder uplift |
Comment 11•8 years ago
|
||
Flagging this for manual testing, instructions (for Nightly) available in Comment 6.
Bob, would it be possible for you to provide a similar testcase for Beta 53 as well? Or perhaps there's a workaround Manual QA could use to adapt the testcase you created for Nightly?
Flags: qe-verify+
Flags: needinfo?(bobowencode)
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #11)
> Flagging this for manual testing, instructions (for Nightly) available in
> Comment 6.
>
> Bob, would it be possible for you to provide a similar testcase for Beta 53
> as well? Or perhaps there's a workaround Manual QA could use to adapt the
> testcase you created for Nightly?
They could flip the browser.tabs.remote.separateFileUriProcess pref to true, to see the same issue with the file content process in aurora and beta.
This at least demonstrates the issue and fix, but the file content process is actually only enabled on Nightly.
I don't have a different scenario that demonstrates the problem without doing that, if I think of something I'll let you know.
Flags: needinfo?(bobowencode)
Comment 13•8 years ago
|
||
Hi,
I have reproduced this issue and I can confirm that it is VERIFIED FIXED in:
- Nightly 55.0a1 (id: 20170406030206)
- Aurora 54.0a2 (id: 20170406004017)
- Beta 53.0b9
I used Windows 10 x64 for testing this.
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•8 years ago
|
status-firefox-esr52:
--- → unaffected
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•