Closed Bug 1351702 Opened 5 years ago Closed 5 years ago

tab-content.js doesn't implement nsIWebBrowserChrome3::shouldLoadURI correctly

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
major

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)

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.
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+
https://hg.mozilla.org/mozilla-central/rev/09c307376824
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?
Attached file file-post-test.html
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.
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+
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)
(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)
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+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.