1513241 - Pass loadURIOptions dictionary for docshell loads

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[Christoph Kerschbaumer [:ckerschb}]

Currently we individually pass a
* triggeringPrincipal
* referrer
* referrer policy

from the front end into docshell. Soon we also need to pass a CSP and most likely we would like to pass more such 'security' args to docshell in the foreseen future.

Instead of having to change all intermediate functions and pass yet another argument we then only have to update fields within that struct.

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #0)
> Currently we individually pass a
> * triggeringPrincipal
> * referrer
> * referrer policy

I just thought of another member for that struct:
* the storagePrincipal, which we would like to use to separate browsing contexts is a good candidate to live in that struct.

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1513241_security_struct.patch

Boris, as discussed at the AllHands it makes sense to pass a 'security struct' from the frontend code into docshell. Before going further down that route though, I wanted to make sure I am not missing anything.

Here is my design:

1) Created a new nsIDocSecArgs.idl which holds triggeringPrincipal, CSP, referrer, referrerPolicy (later also a storagePrincipal).

2) Changed LoadUriWithOptions() as well as LoadURI() to take that new struct from (1) instead of individual arguments.

3) Within DoURILoad() we use the newly passed aCsp instead of querying the CSP from the triggeringPrincipal (but also asserting that they are the same). Please note that it's that CSP argument we can then use within Bug 965637 for new documents.

Obviously there are a few things missing, e.g. before passing the newly created docSecArgs object, I need to push through the CSP to some more API layers, e.g. from tabbrowser.xml. Also, I need to expose an .init() for the docSecArgs and a few other things...

Modulo the above, does that all look acceptable to you or am I missing something?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Created a new nsIDocSecArgs.idl

I'm not sure about the naming there...

And actually, I'm not 100% sure why we want a new XPCOM type here.  Seems like we could just use jsval in the xpidl and convert to a WebIDL dictionary on the C++ side.  That would let JS consumers pass in options objects in a natural way, right?  Do we need a refcounted type on the C++ side, or would having a struct that we can pass around (a WebIDL dictionary) be fine?
 
Apart from that, this seems fine, though.  I'd like Kyle to take a look as well, to see how this works with the changes he's planning.

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)
> > Created a new nsIDocSecArgs.idl
> I'm not sure about the naming there...

The name is an easy fix, if you have any preference, let me know!
 
> And actually, I'm not 100% sure why we want a new XPCOM type here.  Seems
> like we could just use jsval in the xpidl and convert to a WebIDL dictionary
> on the C++ side.  That would let JS consumers pass in options objects in a
> natural way, right?  Do we need a refcounted type on the C++ side, or would
> having a struct that we can pass around (a WebIDL dictionary) be fine?

I think it's not that simple unfortunately, while I agree it would be preferable to generate a webidl dictionariy, there are members within that struct that make this hard. We have nsIPrincipal which would translate to Principal in webidl, so that part is fine, but I don't know what to do about nsIContentSecurityPolicy - is there even a non trivial solution?

Additionally, do we need to care about future members we would like to add to that dictionary?

I am more than happy to incorporate your suggestions but also wanted to make sure that we are not missing anything here.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I don't think this should interrupt the changes I'm making at the moment, as this is a subset of what we later include in nsDocShellLoadState, so we'll probably end up subsuming it in the LoadState creation in LoadURI. We'd need to possible add the storage principal to LoadState, depending on where it's needed in the load.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> but I don't know what to do about nsIContentSecurityPolicy - is there even a non trivial solution?

You can use XPCOM types in webidl.  That's how "Principal" works.  You just put:

  interface Foo;

in your .webidl file to declare the XPCOM interface as an interface and add:

  addExternalIface('Foo', nativeType='nsIContentSecurityPolicy',
                   headerFile='nsIContentSecurityPolicy.h', notflattened=True)

to Bindings.conf to map the name "Foo" to the right XPCOM interface.  You could just use "nsIContentSecurityPolicy" for "Foo", or "ContentSecurityPolicy", or whatever.

> Additionally, do we need to care about future members we would like to add to that dictionary?

I'm not sure what you're asking, exactly...

Comment 7

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #6)
> > but I don't know what to do about nsIContentSecurityPolicy - is there even a non trivial solution?
> 
> You can use XPCOM types in webidl.  That's how "Principal" works.

Ah, I didn't know it's that simple.

> > Additionally, do we need to care about future members we would like to add to that dictionary?
> 
> I'm not sure what you're asking, exactly...

I didn't know we can use XPCOM types in webidl which renders this second part of the question obsolete.

Comment 8

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1513241_security_struct.patch

Boris, just wanted to run this WIP-patch by you real quick to make sure I am not missing anything. Here is what I did:

1 - I created a LoadArgs.webidl file which holds a dictionary for all the arguments.

2 - I updated loadURI() within nsIWebNavigation.idl by the following argument:
      in jsval aLoadArgs,

Using a jsval allows to use .Init() to actually create the dictionary on the C++ side. The downside however is that we have to create a JSVal for all the C++ callers of LoadURI(). There are are a few of C++ callers which now create a JSVal from the dictionary only to then reverse that operation within LoadURI(). Would it be possible to create an overloaded function of LoadURI? One that takes a jsval and the other that takes dom::LoadArgs as an argument so we don't have to deal with JSVals in C++ land. Or alternatively one that is only available within C++ and then we only forward the .idl one to the C++ one?

What do you think?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yes, you could add a loadURI overload that takes the WebIDL dictionary directly.  Something like this, in nsIWebNavigation.idl at the top:

  [ref] native LoadArgs(const mozilla::dom::LoadArgs);

and then in the actual interface definition:


  [implicit_jscontext,binaryname(LoadURIFromScript)]
  void loadURI(in AString                  aURI,
               in unsigned long            aLoadFlags,
               in nsIURI                   aReferrer,
               in jsval                    aLoadArgs,
               in nsIInputStream           aPostData,
               in nsIInputStream           aHeaders,
               [optional] in nsIPrincipal  aTriggeringPrincipal);
               in nsIInputStream           aHeaders);

  // Same thing, but for C++ callers.
  [nostdcall]
  void loadURI(in AString                  aURI,
               in unsigned long            aLoadFlags,
               in nsIURI                   aReferrer,
               in LoadArgs                 aLoadArgs,
               in nsIInputStream           aPostData,
               in nsIInputStream           aHeaders,
               [optional] in nsIPrincipal  aTriggeringPrincipal);
               in nsIInputStream           aHeaders);

The [nostdcall] is not strictly needed, but if you do that you can just have it returning "nsresult" instead of NS_IMETHODIMP.  The binaryname bit _is_ needed, because you don't want them having the same name on the C++ side, since that would make the vtable unpredictable.

Then the jsval version (which in C++ is called LoadURIFromScript) will create a dictionary from the given val and call the dictionary version, and C++ callers can call the dictionary version directly.

Comment 10

[:Gijs (he/him)]

Without wanting to derail this bug, if we're adding a new API anyway, any chance we could move the other pile of arguments on the new struct so the new API just takes a URI plus an options webidl dictionary? It'd make a lot of this type of code a lot more readable (at least in JS).

Comment 11

[Christoph Kerschbaumer [:ckerschb}]

(In reply to :Gijs (he/him) from comment #10)
> Without wanting to derail this bug, if we're adding a new API anyway, any
> chance we could move the other pile of arguments on the new struct so the
> new API just takes a URI plus an options webidl dictionary? It'd make a lot
> of this type of code a lot more readable (at least in JS).

Yeah, I am totally fine with that. Currently the .webidl looks like this:

+dictionary LoadArgs {
+  required Principal? triggeringPrincipal;
+  ContentSecurityPolicy csp;
+  URI referrerURI;
+  long referrerPolicy;
+};

What else would you like to have in there?

Comment 12

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> (In reply to :Gijs (he/him) from comment #10)
> > Without wanting to derail this bug, if we're adding a new API anyway, any
> > chance we could move the other pile of arguments on the new struct so the
> > new API just takes a URI plus an options webidl dictionary? It'd make a lot
> > of this type of code a lot more readable (at least in JS).
> 
> Yeah, I am totally fine with that. Currently the .webidl looks like this:
> 
> +dictionary LoadArgs {
> +  required Principal? triggeringPrincipal;
> +  ContentSecurityPolicy csp;
> +  URI referrerURI;
> +  long referrerPolicy;
> +};
> 
> What else would you like to have in there?

This seems fine for security-related stuff. The other thing is that we should be moving towards passing nsIURIs or webidl URLs to these types of APIs instead of strings, but I dunno how tricky that'd be.

`LoadArgs` is pretty generic, though, and if it's not specific to security then IMO load flags, postData and additional headers (which seem to be referenced in comment #9) could also go in there - basically everything besides the URI.

Comment 13

[Christoph Kerschbaumer [:ckerschb}]

(In reply to :Gijs (he/him) from comment #12)
> `LoadArgs` is pretty generic, though, and if it's not specific to security
> then IMO load flags, postData and additional headers (which seem to be
> referenced in comment #9) could also go in there - basically everything
> besides the URI.

Yeah that makes sense, updating the title of the bug to reflect that change as well.

Comment 14

[Christoph Kerschbaumer [:ckerschb}]

After some intensive hacking I realized that CSP requires more changes. I am suggesting we provide the fundamental architecture for loadArgs within this Bug and do the CSP stuff within Bug 1518454.

Comment 15

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1513241_part_1_update_loadURI_interface.patch

Boris,
let me summarize the changes within this patch:

• We create a LoadArgs.webidl which contains a dictionary for arguments we want to pass to LoadURI().
• We update nsIWebNavigation.idl and pass that new loadArgs object to LoadURIWithOptions and since LoadURI() was just forwarding calls to LoadURIWithOptions() I removed LoadURI completely.
• Probably we can be a little smarter within the WebIDL regarding default values. For now, I only require a triggeringPrincipal and default the loadFlags to 0.
• As mentioned, the CSP bits are getting pretty big sizewise, so let's do all the CSP related changes to the loadArgs dict within Bug 1518454.
• Gijs offered his help to review the frontend bits if you are fine with those backend changes.

Thanks!

Comment 16

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1513241_part_2_frontend_consumers.patch

Gijs,
please see the previous comment for some background. Additionally, I don't like the additional

•  if (postData) {
  

•    loadArgs.postStream = postData;
  

•  }
  

instead of just adding let loadArgs = { ..., postStream: postData }, but if postData is null then I get the following error:

Console message: [JavaScript Error: "'postStream' member of LoadArgs is not an object."

Maybe it has to do with the defaults in the LoadArgs.webidl but I am not sure - would be really happy if we can clean that up.

Thanks!

Comment 17

[:Gijs (he/him)]

You should be able to use the ? suffix for properties on the dictionary that can be null, I think?

In fact, this means I'm a bit confused about the triggering principal:

required Principal? triggeringPrincipal;

Comment 18

[Boris Zbarsky [:bzbarsky]]

Yeah, passing an explicitly null postStream seems fine so we should allow that.

In fact, this means I'm a bit confused about the triggering principal:

We currently have a mode where we allow null to be passed (when the "security.strict_security_checks.enabled" pref is false. That's the default on release, beta, and Android...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9035079 [details] [diff] [review]
bug_1513241_part_1_update_loadURI_interface.patch

>+++ b/docshell/base/nsDocShell.cpp
>+nsDocShell::LoadURIWithOptions(const nsAString& aURI,

Why not just call this loadURI? There is no more "non-options" version, and the fact that it has options is obvious from the options member...

>+                               const mozilla::dom::LoadArgs& aLoadArgs) {

This file has "using namespace mozilla::dom", so I don't think we need the full namespacing here.

>+  nsCOMPtr<nsIInputStream> postStream;
>+  if (aLoadArgs.mPostStream.WasPassed()) {
>+    postStream = aLoadArgs.mPostStream.Value();
>+  }

Since we should allow null for the postStream anyway, with null representing "no post data", the IDL should probably just have:

  InputStream? postStream = null;

and then this code can be:

  nsCOMPtr<nsIInputStream> postStream = aLoadArgs.mPostStream;

>+  if (aLoadArgs.mHeaders.WasPassed()) {

Same thing here; I think we should just allow passing null to represent "no header stream" and set that up as the default value.

>+  if (aLoadArgs.mBaseURI.WasPassed()) {

And probably same here; might as well allow passing null explicitly for the base URI.

I don't know whether we'd still want the headerStream and baseURI locals if we do that.

>+  if (aLoadArgs.mReferrerURI.WasPassed()) {

We probably want to allow passing null here too, actually. Especially since the IDL comments claim something about what happens if it's null. ;)

>+nsDocShell::LoadURIWithOptionsFromScript(const nsAString& aURI,
>+  dom::LoadArgs loadArgs;

Is the namespace prefixing needed?

>@@ -6905,22 +6917,22 @@ nsresult nsDocShell::EndPageLoad(nsIWebP
>+          dom::LoadArgs loadArgs;

Is the prefixing needed?

>+++ b/docshell/base/nsIDocShell.idl
>+interface nsIContentSecurityPolicy;

Hmm. Why is this needed?

>+++ b/docshell/base/nsIWebNavigation.idl
>+#include "mozilla/dom/LoadArgsBinding.h"

Why is this needed? The "native" decl should forward-declare the type, and I doubt the header needs to see the actual declaration...

>+   * @param aLoadArgs
>+   *        A JSObject of the form LoadArgs.webidl holding info like e.g. the

"defined in LoadArgs.webidl", right?

>+++ b/dom/ipc/TabChild.cpp
> +  dom::LoadArgs loadArgs;

Do we need the prefixing?

>+      NS_ConvertUTF8toUTF16(aURI),loadArgs);

Missing space. Did you clang-format this? If not, please do.

>+++ b/dom/security/FramingChecker.cpp
>@@ -252,18 +253,21 @@ static bool ShouldIgnoreFrameOptions(nsI
>+          dom::LoadArgs loadArgs;

Do we need the prefixing?

>+++ b/dom/webidl/LoadArgs.webidl
>+dictionary LoadArgs {

This name seems very generic... How about LoadURIOptions (and rename the .webidl too).

>+   * The principal that initiated the load.
>+   */
>+  required Principal? triggeringPrincipal;

There was more documentation in the .idl. Is leaving it out purposeful?

I'm a bit torn on whether this should be in the dictionary or just an argument, given the "required" status. Especially given the C++ callers, who can easily forget to set the member...

That said, if we're planning to just pass around the entire LoadURIOptions struct as a whole, it makes sense to have this in the dictionary.

Maybe we should add a constructor to dictionaries with required args that takes those args... Maybe file a followup on that and reference this dictionary?

>+   * One of the REFERRER_POLICY_* constants from nsIHttpChannel.
>+   *  Normal case is REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE.

I know you just copied this comment, but seems to be the "normal case" is UNSET and then the internals will do something with that, right?

>+++ b/editor/composer/nsEditingSession.cpp
>+      dom::LoadArgs loadArgs;

Do we need the prefixing?

>+++ b/xpfe/appshell/nsWebShellWindow.cpp
>+    dom::LoadArgs loadArgs;

Do we need the prefixing?

r=me with those bits fixed. Thank you for doing this!

Comment 20

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1513241_part_1_update_loadURI_interface.patch

Thanks Boris, I incorporated all of your suggestions, in particular:

• Renamed the dictionary as well as the file to LoadURIOptions.webidl
• Kept LoadURI() and ditched LoadURIWithOptions()
• Removed namespace prefixing where possible

Regarding the triggeringPrincipal as part of the dictionary or having it as a separate argument

• I prefer having it within the dictionary because as you have pointed out we can pass around the loadURIOptions struct as a whole. Additionally we have strong assertions within LoadURI() which enforce we have a triggeringPrincipal set. Please note that even if the pref security.strict_security_checks.enable is set to false we still assert that LoadURI gets a valid triggeringPrincipal.

Copying over r+ from bz!

Comment 21

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1513241_part_2_frontend_consumers.patch

Gijs, this is way cleaner now having the webidl updated with the default values. What do you think?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Why is this needed? The "native" decl should forward-declare the type, and I doubt the header needs to see the actual declaration...

What happened to that review comment?

Comment 23

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #22)

Why is this needed? The "native" decl should forward-declare the type, and I doubt the header needs to see the actual declaration...

What happened to that review comment?

Sorry for not pointing that out explicitly, but I didn't ignore your comment. If I remove |#include "mozilla/dom/LoadURIOptionsBinding.h"| from nsIWebNavigation.idl I get the following compile error:

0:31.02 /home/ckerschb/source/mc-dbg/dist/include/nsIWebNavigation.h:95:93: error: no type named 'LoadURIOptions' in namespace 'mozilla::dom'
0:31.02 JS_HAZ_CAN_RUN_SCRIPT virtual nsresult LoadURI(const nsAString& aURI, const mozilla::dom::LoadURIOptions & aLoadURIOptions) = 0;

Comment 24

[Boris Zbarsky [:bzbarsky]]

Ah, I see. We don't automatically generate forward-decls for "native" bits.

Still, all we should need here is a forward-decl (inside the "%{C++" section), not an include, right?

Comment 25

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #24)

Ah, I see. We don't automatically generate forward-decls for "native" bits.
Still, all we should need here is a forward-decl (inside the "%{C++" section), not an include, right?

Unfortunately not - I tried several things.
First approach: Simply doing:

%{ C++
class mozilla::dom::LoadURIOptions;
%}

causes this compile error:

In file included from /home/ckerschb/source/mc/uriloader/base/nsDocLoader.cpp:14:
0:04.64 /home/ckerschb/source/mc-dbg/dist/include/nsIWebNavigation.h:38:21: error: no class named 'LoadURIOptions' in namespace 'mozilla::dom'
0:04.64 class mozilla::dom::LoadURIOptions;

Second Approach:
If in nsIWebNavigation.idl I do:

webidl LoadURIOptions;

it produces the following code within dist/include/nsIWebNavigation.h:

namespace mozilla {
namespace dom {
class LoadURIOptions; /* webidl LoadURIOptions */
} // namespace dom
} // namespace mozilla

which then causes this warning.

1:42.28 struct LoadURIOptions : public DictionaryBase
1:42.28 ^
1:42.28 /home/ckerschb/source/mc-dbg/dist/include/nsIWebNavigation.h:39:1: note: did you mean struct here?
1:42.28 class LoadURIOptions; /* webidl LoadURIOptions */
1:42.28 ^~~~~
1:42.28 struct
1:47.07 1 warning generated.

TDIL because I was not aware that we can use forward declarations within *.idl. Do you want me to file a bug to generate forward declarations for native bits? Or how should we proceed?

Comment 26

[Boris Zbarsky [:bzbarsky]]

First approach: Simply doing:

You want:

  %{C++
    namespace mozilla {
    namespace dom {
    struct LoadURIOptions;
    } // namespace dom
    } // namespace mozilla
  %}

Comment 27

[:Gijs (he/him)]

Comment on attachment 9035385 [details] [diff] [review]
bug_1513241_part_2_frontend_consumers.patch

Review of attachment 9035385 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +1075,5 @@
> +        triggeringPrincipal,
> +        loadFlags: flags,
> +        referrerURI,
> +        referrerPolicy,
> +        postStream: postData,

It's kind of unfortunate that we call this postStream consistently on the cpp site and postdata consistently on the js side, but I don't have great solutions apart from just living with this for now...

@@ +1130,5 @@
> +        loadFlags: flags,
> +        referrerURI,
> +        referrerPolicy,
> +        postStream: postData,
> +      };

Nit: just define this once before the try...catch and use it in both branches.

::: toolkit/components/remotebrowserutils/RemoteWebNavigation.js
@@ +92,5 @@
>      }
>  
>      this._sendMessage("WebNavigation:LoadURI", {
>        uri: aURI,
> +      flags: aLoadURIOptions.loadFlags,

Can you file a follow-up issue to make these object prop names match to make this slightly more ergonomic?

Comment 28

[Boris Zbarsky [:bzbarsky]]

I would be fine with calling it postData on the C++ side, fwiw.

Comment 29

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #26)

First approach: Simply doing:

You want:

  %{C++
    namespace mozilla {
    namespace dom {
    struct LoadURIOptions;
    } // namespace dom
    } // namespace mozilla
  %}

Thanks - that works!

Comment 30

[Christoph Kerschbaumer [:ckerschb}]

The code we are having at the moment causes some problems on Android [1], which is also why we only assert that we pass the right triggeringPrincipal for top-level loads if it's not an Android build. To accomodate, I changed the LoadURIOptions.webidl to

Principal? triggeringPrincipal = null;
so the principal is not required. Jonathan will fix that problem within Bug 1504968. I'll leave a comment within Bug 1504968 as well.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=61ab3e53d35dba0a5db9c20394a7000cd301bef7

Comment 31

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #28)

I would be fine with calling it postData on the C++ side, fwiw.

I updated that part and also called it postData in the webidl.

Comment 32

[Christoph Kerschbaumer [:ckerschb}]

(In reply to :Gijs (he/him) from comment #27)

 this._sendMessage("WebNavigation:LoadURI", {
   uri: aURI,

•  flags: aLoadURIOptions.loadFlags,
  

Can you file a follow-up issue to make these object prop names match to make
this slightly more ergonomic?

Thanks Gijs, I incorporated your suggestions and filed Bug 1519365 to get those property names updated.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae543902cda
Update loadURI interface and pass a loadURIOptions dictionary from frontend to docshell loads. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/71cb45922e34
Update frontend consumers of loadURI and pass loadURIOptions dictionary. r=gijs

Comment 34

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/6ae543902cda
https://hg.mozilla.org/mozilla-central/rev/71cb45922e34

Comment 35

[Ben Francis [:benfrancis]]

Any chance you could update the docs on MDN to explain what arguments loadURI() now expects?

I'm trying to fix a regression from this in qbrt which calls loadURI() on startup.

I can see that loadURI() expects an object as the second argument and it seems to require that triggeringPrincipal is set in that object, but I'm not sure what to set it to? If I set it to the system principal using Services.scriptSecurityManager.getSystemPrincipal() then it silently fails.

    browser.loadURI(url, {
      'triggeringPrincipal': Services.scriptSecurityManager.getSystemPrincipal()
    });

I don't suppose you could point me in the right direction?

Comment 36

[:Gijs (he/him)]

(In reply to Ben Francis [:benfrancis] from comment #35)

Any chance you could update the docs on MDN to explain what arguments loadURI() now expects?

Which docs are these? The slug and page itself doesn't make it clear. Is this the one on docshell/webnavigation? <browser> ? Or the global function that we (used to?) have in browser windows? Or something else? They're all different...

Either way, it looks like it wasn't updated for a very very long time... AFAIK no versions of loadURI have taken that set of args for a while now. I'm not convinced we need to fix it now - it'd be better to just mark these bits of XUL documentation as deprecated as I expect other parts of it are similarly outdated/wrong/broken.

I can see that loadURI() expects an object as the second argument and it seems to require that triggeringPrincipal is set in that object, but I'm not sure what to set it to? If I set it to the system principal using Services.scriptSecurityManager.getSystemPrincipal() then it silently fails.

It looks like you're using non-e10s. Is that intentional? I might have broken you in bug 1560178, but in that case the loadURI call should throw NS_ERROR_FAILURE .

Comment 37

[:Gijs (he/him)]

Looking at the qbrt source code, you're calling browser.loadURI, where the params default to an empty object ( https://searchfox.org/mozilla-central/rev/b9041f813de0a05bf6de95a145d4e25004499517/toolkit/content/widgets/browser-custom-element.js#965,989 ), and which then calls the webnav/docshell's loadURI, so AFAICT you end up at https://searchfox.org/mozilla-central/rev/b9041f813de0a05bf6de95a145d4e25004499517/docshell/base/nsDocShell.cpp#3940-3942 (only if the security.strict_security_checks.enabled pref is set to true in your setup), so I don't think these patches should be breaking anything "silently". Ping me on IRC or slack if these comments aren't sufficient to unblock you, but I very much doubt these patches are to blame for any breakage. They also didn't change the arguments to the XUL browser's loadURI, so I'm clearing the devdoc keyword.

Comment 38

[Ben Francis [:benfrancis]]

Which docs are these? The slug and page itself doesn't make it clear. Is this the one on docshell/webnavigation? <browser> ? Or the global function that we (used to?) have in browser windows? Or something else? They're all different...

I got there from the <browser> docs.

Either way, it looks like it wasn't updated for a very very long time... AFAIK no versions of loadURI have taken that set of args for a while now. I'm not convinced we need to fix it now - it'd be better to just mark these bits of XUL documentation as deprecated as I expect other parts of it are similarly outdated/wrong/broken.

If it's deprecated, where is the current documentation for the <browser> element?

It looks like you're using non-e10s. Is that intentional? I might have broken you in bug 1560178, but in that case the loadURI call should throw NS_ERROR_FAILURE .

To be clear, I didn't write any of this code, I'm just trying to get it working again. I don't know whether e10s is enabled or not, but my understanding is that qbrt just downloads a Nightly build of Firefox to use as a runtime so I'd expect it to do what ever Nightly does.

The error with the current set of arguments is:

JavaScript error: chrome://global/content/elements/browser-custom-element.js, line 874: TypeError: aParams is null

If I just pass the URL and an empty object for aParams then I get:

JavaScript error: chrome://global/content/elements/browser-custom-element.js, line 886: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.loadURI]

If I set triggeringPrincipal to the system principal in the aParams object, qbrt fails to start but without any errors.

I can't access bug 1560178 so I don't know whether that's relevant.

They also didn't change the arguments to the XUL browser's loadURI, so I'm clearing the devdoc keyword.

Ah sorry, I must have misunderstood what was changed in this patch (I was pointed here as a possible source of the change). The description seemed to match the change in interface on the browser element, but if this is unrelated I'll try to find you in IRC/Slack rather than continue the conversation here.