Closed Bug 1379369 Opened 3 years ago Closed 2 years ago

Port Bug 1364016 [Have _openURIInNewTab() provide the correct triggeringPrincipal] to SeaMonkey

Categories

(SeaMonkey :: Tabbed Browser, defect, major)

defect
Not set
major

Tracking

(seamonkey2.53 affected, seamonkey2.54 fixed)

RESOLVED FIXED
seamonkey2.54
Tracking Status
seamonkey2.53 --- affected
seamonkey2.54 --- fixed

People

(Reporter: frg, Assigned: frg)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 12 obsolete files)

2.12 KB, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
30.50 KB, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
8.69 KB, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
10.83 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
32.03 KB, patch
frg
: review+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
9.36 KB, patch
frg
: review+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
1.09 KB, patch
frg
: review+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
1.10 KB, patch
Details | Diff | Splinter Review
Passing urls to SeaMonkey from the desktop or another programm no longer works. A blank browser tab is opened. Now a triggeringPrincipal needs to be provided.
The Duplicate This Tab extension might need an upgrade. I'm adding its author to the CC and now I'll post a support request with the extension.
Duplicate of this bug: 1380566
Duplicate of this bug: 1382543
Attached patch 1379369-wip.patch (obsolete) — Splinter Review
Seems to do the jobs but still needs cleanups (e.g. replace param either implemented or dropped from patch) and maybe further changes.
Attached patch 1379369-wip2.patch (obsolete) — Splinter Review
Done, I hope.

Need to adjust the comment before asking for review.
Attachment #8898008 - Attachment is obsolete: true
Depends on: 1391665
Attached patch 1379369-wip-beta.patch (obsolete) — Splinter Review
A more complete fix but we are still missing principal and referrer usage in quite a few functions. I think this one is it and I will do followup patches before it gets out of hand.

Fixes issues covered in Bug 1379842, Bug 1113431 and Bug 1364016 but we have so many different tab routines that fixing all of it is like an onion and beyond the scope of this bug. I left a few TODOs in the code.

Bug 1379842 changed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9de512d55bb4
Attachment #8898511 - Attachment is obsolete: true
Would it be easier to port Bug 1113431 first, then do other work on top of that?
Flags: needinfo?(frgrahl)
Attached patch 1379369-1-groundwork.patch (obsolete) — Splinter Review
Base patch to get SeaMonkey at least to a starting level for a port to be possible. It contains a few formatting changes for the later patches. I can seperate them if desired.
Attachment #8899196 - Attachment is obsolete: true
Flags: needinfo?(frgrahl)
Attachment #8900297 - Flags: review?(iann_bugzilla)
Base patch for current comm-beta.
Port of Bug Port Bug 1364016. Some parts are already in c-c.
Attachment #8900299 - Flags: review?(iann_bugzilla)
Port Bug 1364016 for current comm-beta
Port Bug 1379369
Attachment #8900301 - Flags: review?(iann_bugzilla)
Attached patch 1379369-4-referrerpolicy.patch (obsolete) — Splinter Review
Port of Bug Bug 1113431. This needs further work as do the principal use.

I usgesst you test all 4 parts on Beta. 2.54 is currently mostly broken.
Attachment #8900302 - Flags: review?(iann_bugzilla)
Attached patch 1379369-1-groundwork.patch (obsolete) — Splinter Review
Rebased patch.
Attachment #8900297 - Attachment is obsolete: true
Attachment #8900297 - Flags: review?(iann_bugzilla)
Attachment #8901982 - Flags: review?(iann_bugzilla)
Rebased patch. Filename also changed.
Attachment #8900299 - Attachment is obsolete: true
Attachment #8900299 - Flags: review?(iann_bugzilla)
Attachment #8901984 - Flags: review?(iann_bugzilla)
rebased comm-beta patch
Attachment #8900298 - Attachment is obsolete: true
rebased comm-beta patch
Attachment #8900300 - Attachment is obsolete: true
Comment on attachment 8901982 [details] [diff] [review]
1379369-1-groundwork.patch

>+++ b/suite/browser/navigator.js
>@@ -394,31 +394,36 @@ nsBrowserAccess.prototype = {
>                             ? aOpener.document.nodePrincipal.originAttributes.userContextId
>                            : Components.interfaces.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID;
>         let openerWindow = (aFlags & nsIBrowserDOMWindow.OPEN_NO_OPENER) ? null : aOpener;
> 
>         var newTab = gBrowser.loadOneTab(uri, {inBackground: bgLoad,
>                                                fromExternal: isExternal,
>                                                relatedToCurrent: isRelated,
>                                                referrerURI: referrer,
>-                                               userContextId: userContextId,
>+                                               userContextId,
This change doesn't look correct, the Firefox equivalent still has as "userContextId: userContextId,".

>+          if (aURI) {
>+            gBrowser.loadURIWithFlags(aURI.spec, {
>+                                                  flags: loadflags,
>+                                                  referrerURI: referrer,
>+                                                  userContextId,
I cannot see anything in tabbrowser.xml that accepts userContextId or am I missing something?

I think some changes to tabbrowser.xml are also needed to include userContextId.

f+ for the moment
Attachment #8901982 - Flags: review?(iann_bugzilla) → feedback+
I changed the assignment but it should not be necessary because the name is the same. Just uses a new reference in the params instead of the original object so I thought I could save a few cpu cycles.

Making user contexts actually work in SeaMonkey is a project for itself but unless private browsing is replaced with a context I see no need to do so:

https://wiki.mozilla.org/Security/Contextual_Identity_Project/Containers

I just put them in to ease further porting and make any checks happy. Most apis now take the parameter. SeaMonkey always uses the default user context for everything. You are better off using a different profile if you need the functionality.

Beta patch not updated for now.
Attachment #8901982 - Attachment is obsolete: true
Attachment #8903925 - Flags: review?(iann_bugzilla)
rebased
Attachment #8901984 - Attachment is obsolete: true
Attachment #8901984 - Flags: review?(iann_bugzilla)
Attachment #8903926 - Flags: review?(iann_bugzilla)
rebased
Attachment #8900302 - Attachment is obsolete: true
Attachment #8900302 - Flags: review?(iann_bugzilla)
Attachment #8903927 - Flags: review?(iann_bugzilla)
Comment on attachment 8903925 [details] [diff] [review]
1379369-1-groundwork-V2.patch

LGTM r=me
Attachment #8903925 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8903926 [details] [diff] [review]
1379369-2-triggeringprincipal-V2.patch

LGTM r=me
Attachment #8903926 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8900301 [details] [diff] [review]
1379369-3-browserdraganddrop.patch

LGTM r=me
Attachment #8900301 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8903927 [details] [diff] [review]
1379369-4-referrerpolicy-V2.patch

LGTM r=me
Attachment #8903927 - Flags: review?(iann_bugzilla) → review+
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/008b94f56181
Part 1. Base patch passing a triggeringPricipal and userContextId to more parts in the tabbrowser. r=IanN
https://hg.mozilla.org/comm-central/rev/921b8e3b2731
Part 2. Port Bug 1364016 [Explicitly pass a triggeringPrincipal to openURI] to SeaMonkey. r=IanN
https://hg.mozilla.org/comm-central/rev/7bdcbbdd0e1f
Part 3. Port Bug 1379842 [Check content principal when dragging and dropping from browser] to SeaMonkey. r=IanN
https://hg.mozilla.org/comm-central/rev/a202c95f898c
Part 4. Port part of Bug 1113431 [Propagate referrer policy throughout the UI] to SeaMonkey. r=IanN
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.54
Rebased patch for 2.53 comm-beta. r+ from IanN carried forward.

[Approval Request Comment]
Regression caused by (bug #): Bug 1364016
User impact if declined: external url handling broken.
Testing completed (on m-c, etc.): c-b
Risk to taking this patch (and alternatives if risky): none broken already.
String changes made by this patch: --
Attachment #8901995 - Attachment is obsolete: true
Attachment #8906402 - Flags: review+
Attachment #8906402 - Flags: approval-comm-beta?
Rebased patch for 2.53 comm-beta. r+ from IanN carried forward.

[Approval Request Comment]
Regression caused by (bug #): Bug 1364016
User impact if declined: external url handling broken.
Testing completed (on m-c, etc.): c-b
Risk to taking this patch (and alternatives if risky): none broken already.
String changes made by this patch: --
Attachment #8901997 - Attachment is obsolete: true
Attachment #8906403 - Flags: review+
Attachment #8906403 - Flags: approval-comm-beta?
Comment on attachment 8903927 [details] [diff] [review]
1379369-4-referrerpolicy-V2.patch

[Approval Request Comment]
Regression caused by (bug #): Bug 1364016
User impact if declined: external url handling broken.
Testing completed (on m-c, etc.): c-b
Risk to taking this patch (and alternatives if risky): none broken already.
String changes made by this patch: --

Part 3 is 2.54+ and must not be checked into Beta.
Attachment #8903927 - Flags: approval-comm-beta?
Seamonkey Build identifier: 20170911061039 is still broken. If a browser window is already open, links from 3rd party apps always open a new window and show the home page regardless of pref settings.
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/7fe78c5eaf64
Part 5. Remove undefined UserContextID in default open uri path. rs=bustage-fix
r+ bustage fix.

In one case the UserContextId was undefined. Pre 2.54 didn't care but recent changed exposed the bug. See screenshot in Comment 30.

Rather than refactoring the patches for 2.53 I would like them to match the ones in 2.54 and push the bustage fixe there too if approved.
Attachment #8907069 - Flags: review+
Attachment #8907069 - Flags: approval-comm-beta?
K Chayka would you like to test a 2.53 or 2.54 version with all the work patches still in review applied? I tested this for 2.53 a few weeks ago and it did work for me. Might be new breakage. If not I will see that I check it over the weekend.
Flags: needinfo?(bugZ)
frg: sure - I'd be glad to test a 2.54 build for Win64. Where can I get one?
Flags: needinfo?(bugZ)
Comment on attachment 8903927 [details] [diff] [review]
1379369-4-referrerpolicy-V2.patch

a=me
Attachment #8903927 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 8906402 [details] [diff] [review]
1379369-1-groundworkbeta-V2-253.patch

a=me
Attachment #8906402 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 8906403 [details] [diff] [review]
1379369-2-triggeringprincipal-V2-253.patch

a=me
Attachment #8906403 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 8907069 [details] [diff] [review]
1379369-5-userContextId-fix.patch

a=me
Attachment #8907069 - Flags: approval-comm-beta? → approval-comm-beta+
Duplicate of this bug: 1398497
Comment on attachment 8900301 [details] [diff] [review]
1379369-3-browserdraganddrop.patch

Just for reference. If someone builds 2.53 from a 56 release tree this one needs to go in. Bug 1379842 was backported to Ggecko 56 and I also made a mistake in part 1 where I left the triggeringPrincipal in the code and didn't intialize it.
No longer blocks: 2.56BulkMalfunctions
Another update for 2.53 only. 

Links with target="_blank" did ignore the configured link behaviour and open in a new window. Bad backport for 2.53 only. The removed lines are correct for 2.54 and up so this does not need a followup bug. 

An example is clicking on a release notes link here: https://www.visualstudio.com/downloads/
Blocks: 1463082
Blocks: 1479191
You need to log in before you can comment on or make changes to this bug.