Make window.arguments handling in browser windows less crazy

NEW
Unassigned
(NeedInfo from)

Status

()

enhancement
P5
normal
9 months ago
9 months ago

People

(Reporter: Gijs, Unassigned, NeedInfo)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 affected)

Details

Reporter

Description

9 months ago
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?
Flags: needinfo?(mconley)
Flags: needinfo?(jkt)
Flags: needinfo?(hsivonen)

Comment 1

9 months ago
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.
> 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
Flags: needinfo?(jkt)
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.
Flags: needinfo?(hsivonen) → needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(mconley)
Reporter

Comment 6

9 months ago
(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?
Flags: needinfo?(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?
Flags: needinfo?(mconley) → needinfo?(gijskruitbosch+bugs)

Comment 8

9 months ago
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.
See Also: → 1393570
Reporter

Comment 9

9 months ago
(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.
You need to log in before you can comment on or make changes to this bug.