1485961 - Make window.arguments handling in browser windows less quixotic and labyrinthine

Status: NEW

(Firefox :: General, enhancement, P5)

Description

[:Gijs (he/him)]

Handling code in browser.js:
https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/browser/base/content/browser.js#1647-1705

Sending code in nsBrowserContentHandler (probably not the only consumer):
https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/browser/components/nsBrowserContentHandler.js#178-252

As far as I can tell, we support:

- passing a tab XUL element to "adopt"
- passing a single string that has one or more URIs, separated by `|` characters. This is always loaded with system principal... This is used by the user's homepage pref, and thus by browser content handler's `defaultArgs`
- passing an array of arguments, one of which is a URI and the rest information about loading that URI
-- we use the length of this array (3 or greater) to determine whether we do the `|` splitting. That feels pretty yucky.
-- some of this information seems like it should be removed. In particular, AFAICT the charset is always ignored (:hsivonen, can you check me on that?), and the userContextId seems like something we should be getting off the principal (:jkt, I know we can't do that for system principal. Under what circumstances do we pass system principal *and* a usercontextid through window.arguments ?)
-- code that explicitly passes the umpteen parameters as null is ugly. We should switch to some kind of dictionary object to make the code less horrid to read.
- passing an array where the first item is itself an nsIArray, which we then treat as a list of URIs to load, again always with system principal... (we seem to ignore the other items in window.arguments in this case)


I'm not sure what our restrictions are for making this less terrible. I haven't looked, for instance, at what C++ consumers we have. But ideally I'd like a single format for passing items. Something like passing a single first argument to `window.arguments` that looks something like:

{
  tabToAdopt: null-or-tab,
  uriList: array of 1 or more URIs to load,
  triggeringPrincipal, postData, referrer, ... everything else that goes in the array right now.
}

The `defaultArgs` code will be responsible for turning the `|`-separated thing into an array instead.

:mconley, any idea if that's feasible, or if there's a reason we need this stuff to be the way it is due to our C++-side window opening code?

Comment 1

[Tim Nguyen :ntim]

In bug 1444760, I got rid of the charset parameter handling for loadURI/loadURIWithFlags, since it seemed unused, and we haven't noticed any regression since.

Comment 2

[Jonathan Kingston [:jkt] he/him]

> I know we can't do that for system principal. Under what circumstances do we pass system principal *and* a usercontextid through window.arguments

I don't think we do this anymore with the changes I have been doing to openLinkIn.

However I threw some code at try to verify we could remove this. That however perhaps might fix all usecases in future that currently aren't using the right userContextId when they should and currently use System principal however I think this would be fixable.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb2acd8d4b1bc7dcfbdf8d4be2ff48b536347869

Comment 3

[Henri Sivonen (:hsivonen)]

I'm not really familiar with this code, but window.arguments doesn't look like a Web-exposed feature, so I'm guessing we could change it.

I expect smaug to have a better idea of the constraints here.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

arguments is for [ChromeOnly] openDialog, and the property on window gets then
added in https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/dom/base/nsGlobalWindowInner.cpp#2118-2124
But that all really should happen on chrome windows only.

I'm not familiar with what arguments firefox UI code passes and why.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Yeah, as I understand it, arguments has been a slowly-rotting dumping ground for all sorts of things that need to be passed to a new window, and probably is a bit tortured to maintain backwards compatibility with legacy add-ons.

Passing a dictionary to openDialog seems like a righteous thing to do - and something that I believe WebIDL allows us to define with some strictness: https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/dom/chrome-webidl/WebExtensionPolicy.webidl#174-196

So, feasible? I will cautiously say yes! And is there a good reason to keep the old way of doing things? I can't think of one.

Comment 6

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #5)
> Passing a dictionary to openDialog seems like a righteous thing to do - and
> something that I believe WebIDL allows us to define with some strictness:
> https://searchfox.org/mozilla-central/rev/
> e126996d9b0a3d7653de205898517f4f5b632e7f/dom/chrome-webidl/
> WebExtensionPolicy.webidl#174-196

Right... this would work for window.openDialog, but a bunch of places use the window watcher's openWindow method, which is XPCOM-based. Do we have a similar solution for XPIDL?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

If my understanding of https://groups.google.com/forum/#!searchin/mozilla.dev.platform/Using$20WebIDL$20objects$20in$20XPIDL%7Csort:date/mozilla.dev.platform/uwfhoVi7r98/m_ze0Uz4CQAJ and bug 1444991 is correct, I believe we should be able to define the arguments as a dictionary in WebIDL, and then import them into the nsIWindowWatcher XPIDL definitions.

I'm not sure if I've answered the question... have I?

Comment 8

[Rob Wu [:robwu]]

I'm working on bug 1393570, which needs to pass in the userContextId (window.arguments[6] at the moment). That code needs to be updated if the proposal in this bug is implemented. I've added tests, so there will be test failures if that is forgotten.
If this bug is fixed before mine, then I'll update the code.

window.arguments[1] is currently unused: I traced the code history and found that the last use was removed in bug 871161.

Comment 9

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #7)
> If my understanding of
> https://groups.google.com/forum/#!searchin/mozilla.dev.platform/
> Using$20WebIDL$20objects$20in$20XPIDL%7Csort:date/mozilla.dev.platform/
> uwfhoVi7r98/m_ze0Uz4CQAJ and bug 1444991 is correct, I believe we should be
> able to define the arguments as a dictionary in WebIDL, and then import them
> into the nsIWindowWatcher XPIDL definitions.
> 
> I'm not sure if I've answered the question... have I?

Yeah, I think that helps. I'll try to have a look at this, but I'm not sure I'll have cycles - it depends how heavy the fallout from a change like this ends up being. Keeping ni for now.

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Gijs, another fine bug for your consideration 🍻

Comment 11

[Tom Schuster]

I have been looking into this. And I am wondering if we could nsIOpenWindowInfo for this. It looks like it's already passed in from a lot of callers (at least in C++). I think for openLinkIn we would need some way of creating the interface from JS. Afterwards we could just add hasValidTransientUserGestureActivation to the interface in bug 1738694.

Comment 12

[:Gijs (he/him)]

(In reply to Tom Schuster (MoCo) from comment #11)

I have been looking into this. And I am wondering if we could nsIOpenWindowInfo for this. It looks like it's already passed in from a lot of callers (at least in C++). I think for openLinkIn we would need some way of creating the interface from JS.

Yes.

Afterwards we could just add hasValidTransientUserGestureActivation to the interface in bug 1738694.

I think this would potentially work, but I think there might be some places right now that assume that only C++ sends these objects and make decisions as a result (cf. https://searchfox.org/mozilla-central/rev/b72e9d7d63bf499d1d8168291b93d4ec7fde236e/browser/base/content/tabbrowser.js#2836-2839 ). So we'd have to be a little careful (maybe audit use of this prop in frontend code right now :-( )

But I think an ideal end-state here would be that we get rid of the magic-index-into array-based window.arguments handling.

To do that, I think we'd need a way to include also stuff that's currently separate, like the actual URL to load, into the object. Does that seem feasible?

The other thing I'm wondering is if we'd then pass an array of these objects for cases where we want to open more than one URL... Nika/Tom, wdyt?

Comment 13

[Tom Schuster]

To do that, I think we'd need a way to include also stuff that's currently separate, like the actual URL to load, into the object. Does that seem feasible?

I think that is probably something Nika needs to decide. It would probably be confusing to include the URL into that object, because we already pass the URL as an argument in C++ separately to functions using nsIOpenWindowInfo.

Passing an array of info objects should be possible in theory.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

In general nsIOpenWindowInfo is designed with the goals of the backend in mind, as it tries to effectively act as a way to pass an object into the frontend, which will create a new tab or window for us, and then get some action to happen in C++ when the <browser> element which the info is attached to is created (either attaching an existing BrowserParent for a content-process window.open, or calling the BrowsingContextReadyCallback). It's not been designed around the idea of actually starting a navigation from the frontend using it, code which does that does so by starting the navigation from the nsIBrowsingContextReadyCallback. It's also generally designed for opening content windows, rather than for opening chrome windows, as we also use it when creating a window in a new tab.

My general expectation is that that isn't exactly what frontend wants for arguments to pop-up windows with window.arguments, and I worry a bit that combining both sets of requirements together, we'll make something more confusing. I expect that frontend probably has other flags which aren't related to the content document being loaded in the new window which they want to pass (e.g. window state for the browser window, or you may want to just open a new window with default URLs?), and probably handles navigations differently (IIRC because of how you're called, you generally have the newly opened window start the navigation, rather than starting it from something like a callback).

Perhaps an approach might be to change consumers of window.arguments to instead always pass a single argument which is an object with the properties frontend wants to depend on? We could perhaps change how the backend works such that window.arguments (and the "arguments" argument to nsIWindowWatcher::openWindow) is just the object directly in that case (perhaps requiring some signaling at first), rather than an array-like object, and eventually rip out the nsIArray stuff? Not sure whether that would fix the issues you're looking to fix or not.

Comment 15

[Tom Schuster]

As an intermediate solution, I have been experimenting with adding a nsIPropertyBag as the new last argument. We could then add existing or new arguments to the bag instead and slowly move to something else.

Comment 16

[Tom Schuster]

I bug 1738694 I now added the nsIPropertyBag parameter that I suggested. We could now start moving parameters from the call into this object.

Comment 17

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

nsIPropertyBag seems still less strictly typed to me, it's still a map of random things, one still needs to remember the name and type of the property (although no need to remember property index too, which is good).

What could be even nicer is to get an actually typed struct so that the caller doesn't have to remember the parameter names and types either.