1475699 - Shift-clicked external links from Facebook Container now fail to open (spawn a window with a blank tab), and trigger "Error: IDL methods marked with [implicit_jscontext] or [optional_argc] may not be implemented in JS"

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

The behavior from bug 1449383's STR changed in the past couple of nightlies, to the point that I think a new bug is merited (since it's a notable regression).

STR:

 1. Install the "Facebook Container" add-on:
  https://addons.mozilla.org/en-US/firefox/addon/facebook-container/

 2. Visit https://www.facebook.com/ and log in.

 3. Visit e.g. https://www.facebook.com/mozilla/posts/10156136938527381 (or just scroll down your feed to find a post an external link)

 4. Shift-click the external link (e.g. the "Learn more and APPLY: https://blog.mozilla.org/[...]" textual link in that Mozilla facebook post.)

ACTUAL RESULTS:
A new window opens, with a single tab at about:blank.  Additionally, this appears in my browser console:
> Error: IDL methods marked with [implicit_jscontext] or [optional_argc]
> may not be implemented in JS        browser.js:1039:9

This error is pointing to this "browser.webNavigation.setOriginAttributesBeforeLoading" call in our internal browser.js file -- this line in DXR:
https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/base/content/browser.js#1030

EXPECTED RESULTS:
New window should open, viewing the link that I shift-clicked.


Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=acc0d8ae4c88ad5978714b8b2344d02034dce24d&tochange=30e2bfed294438984fab63f434210906f4c14943

--> regression from bug 1474835

(Before the regression, we already had kinda-weird behavior, as described in bug 1449383 [though I did end up seeing the page content that I clicked].  So it's possible we were already doing something shady here, and the "regression" could've just been that we got stricter in a way that made our preexisting shadiness more-severe. Not sure, just one possibility.)

Comment 1

[Daniel Holbert [:dholbert]]

It looks like bug 1474835 did annotate this "setOriginAttributesBeforeLoading" function with [implicit_jscontext], BTW:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/30e2bfed2944#l2.12

So, if that function has a JS implementation somewhere [and I guess it does], then it makes sense that we started getting a browser console error when bug 1474835 landed.

I don't know which JS implementation is in play here, but FWIW I did find what seems to be three different JS implementations (.js/.jsm files that have stuff about Ci.nsIWebNavigation and have a "setOriginAttributesBeforeLoading(originAttributes) {...}" function block). Specifically these ones:

https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/devtools/client/responsive.html/browser/web-navigation.js#87

https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/toolkit/content/browser-child.js#357

https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/toolkit/components/remotebrowserutils/RemoteWebNavigation.js#112


Based on the Browser Console error message ("may not be implemented in JS"), I'm guessing those are all problematic now, as of bug 1474835...  jandem, do you know what's going on here?

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> STR:
> 
>  1. Install the "Facebook Container" add-on:
>   https://addons.mozilla.org/en-US/firefox/addon/facebook-container/
> 
>  2. Visit https://www.facebook.com/ and log in.

Also, I just noticed -- you don't actually need to log in to facebook, to trigger this bug. So you can skip STR step 2.

Just install facebook container extension and then directly go to https://www.facebook.com/mozilla/posts/10156136938527381 and shift-click the "Learn more and APPLY" link.

(If Facebook gives you a login popup at the /mozilla/posts/... page, just click their "Not now" link to dismiss it.)

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Support invoking JS-implemented functions marked as [implicit_jscontext]

I didn't know [implicit_jscontext] shouldn't be used when there are JS implementations. It looks like this check was added in bug 809674 to fix crashes, but it's actually not too hard to support and might be useful elsewhere too.

The patch implements this and adds a number of xpcshell tests (maybe the test files should be renamed; I can do that if you think it's better).

I verified this is orange on all platforms without the changes to the platform-specific stubs but green with these changes.

This fixes the Facebook Container issue for me.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8992160 [details] [diff] [review]
Support invoking JS-implemented functions marked as [implicit_jscontext]

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

Generally seems ok. I'm a bit concerned about performance, but hopefully it should all be branch-predicted away, and this path isn't exactly fast to start with.

I think I'd probably prefer to MOZ_CRASH on any of the xptcall impls that aren't run on CI, but could be convinced otherwise.

Delegating the review of the precise mechanics to Andrew.

Comment 5

[Andrew McCreight (out until 2026) [:mccr8]]

Comment on attachment 8992160 [details] [diff] [review]
Support invoking JS-implemented functions marked as [implicit_jscontext]

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

::: xpcom/reflect/xptcall/md/unix/xptcstubs_ipf32.cpp
@@ +53,5 @@
>      const nsXPTParamInfo& param = info->GetParam(i);
>      const nsXPTType& type = param.GetType();
>      nsXPTCMiniVariant* dp = &dispatchParams[i];
>  
> +    MOZ_CRASH("Need to skip an argument if IsContextPassedBeforeParam(i)");

What's going on here? It looks like this is just going to make these files completely broken. Did you mean to fix this or what? Also, you should mention this bug number somewhere so that somebody trying to fix this will more easily be able to figure out the context.

::: xpcom/reflect/xptcall/md/unix/xptcstubs_linux_alpha.cpp
@@ +45,5 @@
>          const nsXPTParamInfo& param = info->GetParam(i);
>          const nsXPTType& type = param.GetType();
>          nsXPTCMiniVariant* dp = &dispatchParams[i];
>  
> +        if (info->IsContextPassedBeforeParam(i))

I would complain about the lack of bracing in these files, but the existing style seems to be a complete mess anyways.

::: xpcom/reflect/xptcall/md/unix/xptcstubs_pa32.cpp
@@ +57,5 @@
>      const nsXPTType& type = param.GetType();
>      nsXPTCMiniVariant* dp = &dispatchParams[i];
>  
> +    if (info->IsContextPassedBeforeParam(i)) {
> +      args--;

I would think that this should not do args--, because I would think that the JS context argument would act like a !type.IsArithmetic(). Although I think it is unlikely anybody is still building Firefox for HP-PA RISC 32 bit mode.

::: xpcom/reflect/xptinfo/xptinfo.h
@@ +423,5 @@
>      return mHasRetval ? &Param(mNumParams - 1) : nullptr;
>    }
>  
> +  // Returns true iff this is an [implicit_jscontext] method and param aIndex
> +  // immediately follows the JSContext* argument.

It is unfortunate to have this logic that almost duplicates the logic in CallMethodHelper::InitializeDispatchParams(). I can't think of a way around that without rewriting these dozens of bits of stub code, though. You should refer to the call method helper function in a comment here, and refer to this method in that helper code. You could also add an assertion, maybe like |wantsJSContext ? IsContextPassedBeforeParam(mJSContextIndex + 1) : !IsContextPassedBeforeParam(999)|, to CallMethodHelper::InitializeDispatchParams().

@@ +425,5 @@
>  
> +  // Returns true iff this is an [implicit_jscontext] method and param aIndex
> +  // immediately follows the JSContext* argument.
> +  bool IsContextPassedBeforeParam(uint8_t aIndex) const {
> +    MOZ_ASSERT(aIndex < mNumParams);

You should assert !WantsOptArgc() here, because this method will produce the wrong value in that case. (Of course, that should only be true for methods without a JS implementation.)

Comment 6

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> ::: xpcom/reflect/xptcall/md/unix/xptcstubs_ipf32.cpp
> @@ +53,5 @@
> >      const nsXPTParamInfo& param = info->GetParam(i);
> >      const nsXPTType& type = param.GetType();
> >      nsXPTCMiniVariant* dp = &dispatchParams[i];
> >  
> > +    MOZ_CRASH("Need to skip an argument if IsContextPassedBeforeParam(i)");
> 
> What's going on here? It looks like this is just going to make these files
> completely broken. Did you mean to fix this or what? Also, you should
> mention this bug number somewhere so that somebody trying to fix this will
> more easily be able to figure out the context.

I think the idea is that we have lots of xptcall implementations for unsupported platforms, and it's better to make it clear that they don't work anymore until this is fixed. I agree the message should be clearer.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> What's going on here? It looks like this is just going to make these files
> completely broken. Did you mean to fix this or what? Also, you should
> mention this bug number somewhere so that somebody trying to fix this will
> more easily be able to figure out the context.

Yeah what bholley said - lots of tier 3 platforms and most of them probably don't even have a Rust/LLVM backend. I'll mention the bug number though. It might be nice at some point to remove all tier 3 stubs and add them back only when somebody complains.

> I would complain about the lack of bracing in these files, but the existing
> style seems to be a complete mess anyways.

Yeah I tried to follow the style of the rest of the file...

> I would think that this should not do args--, because I would think that the
> JS context argument would act like a !type.IsArithmetic(). Although I think
> it is unlikely anybody is still building Firefox for HP-PA RISC 32 bit mode.

Each loop iteration does a --args so I think it's needed. But I should have just put this in the MOZ_CRASH bucket - not worth spending more time on this.

> > +  // Returns true iff this is an [implicit_jscontext] method and param aIndex
> > +  // immediately follows the JSContext* argument.
> > +  bool IsContextPassedBeforeParam(uint8_t aIndex) const {
> > +    MOZ_ASSERT(aIndex < mNumParams);
> 
> You should assert !WantsOptArgc() here, because this method will produce the
> wrong value in that case. (Of course, that should only be true for methods
> without a JS implementation.)

This is annoying to do because we throw the "[optional_argc] not supported on JS implementations" at a later point, when we call into XPConnect...

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> It is unfortunate to have this logic that almost duplicates the logic in
> CallMethodHelper::InitializeDispatchParams(). I can't think of a way around
> that without rewriting these dozens of bits of stub code, though. You should
> refer to the call method helper function in a comment here, and refer to
> this method in that helper code. You could also add an assertion, maybe like
> |wantsJSContext ? IsContextPassedBeforeParam(mJSContextIndex + 1) :
> !IsContextPassedBeforeParam(999)|, to
> CallMethodHelper::InitializeDispatchParams().

What if we add |uint32_t IndexOfJSContext() const|, returning UINT32_MAX if !WantsContext (better suggestions welcome), and then:

(1) Add a call to that before the loop in each stub + inside the loop compare |i == indexOfJSContext| (slightly faster too)
(2) Call that in CallMethodHelper::InitializeDispatchParams as well.

I wasn't aware of the very similar code in InitializeDispatchParams - I agree it's nice to deduplicate and I don't mind updating the stubs again, it's a very mechanical change.

I can post an updated patch tomorrow.

Comment 9

[Andrew McCreight (out until 2026) [:mccr8]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> Each loop iteration does a --args so I think it's needed. But I should have
> just put this in the MOZ_CRASH bucket - not worth spending more time on this.

I agree.

> This is annoying to do because we throw the "[optional_argc] not supported
> on JS implementations" at a later point, when we call into XPConnect...

Ah, yeah that's annoying. I guess with the change you suggest in the next comment this doesn't matter.

Comment 10

[Andrew McCreight (out until 2026) [:mccr8]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> What if we add |uint32_t IndexOfJSContext() const|, returning UINT32_MAX if
> !WantsContext (better suggestions welcome), and then:

To match mJSContextIndex, it looks like the return type should be uint8_t, and return UINT8_MAX if !WantsContext.

> (1) Add a call to that before the loop in each stub + inside the loop
> compare |i == indexOfJSContext| (slightly faster too)
> (2) Call that in CallMethodHelper::InitializeDispatchParams as well.

That sounds good. That's exactly what I was thinking of.

> I wasn't aware of the very similar code in InitializeDispatchParams - I
> agree it's nice to deduplicate and I don't mind updating the stubs again,
> it's a very mechanical change.

Thanks for offering to do that. I actually just came across that function when I was trying to figure out how this JSContext index was supposed to work. Both your code and the existing code had a comment on the getter/setter case, so I assumed you'd seen it. I see now the comments aren't actually the same. Where did you look to figure out the logic for the JSContext index, out of curiosity?

> I can post an updated patch tomorrow.

Great.

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Andrew McCreight [:mccr8] from comment #10)
> I see now the comments
> aren't actually the same. Where did you look to figure out the logic for the
> JSContext index, out of curiosity?

Being new to all this, the wiki was pretty helpful and it mentions this:

> Finally, as an exception to everything already mentioned, for attribute getters and setters the JSContext *cx comes before any other arguments.

[0] https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL:

Comment 12

[Bobby Holley (:bholley)]

If either of you wants to write a patch to just nuke the impls we don't test on CI, I think we should do it. We can add them back if/when somebody complains, and we'll get rid of a lot of the ones where it's unclear.

Comment 13

[Andrew McCreight (out until 2026) [:mccr8]]

(In reply to Bobby Holley (:bholley) from comment #12)
> If either of you wants to write a patch to just nuke the impls we don't test
> on CI, I think we should do it. We can add them back if/when somebody
> complains, and we'll get rid of a lot of the ones where it's unclear.

I think that's a little overly broad, but we could surely thin out the list a little. I filed bug 1476121 for that.

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Support invoking JS-implemented functions marked as [implicit_jscontext]

As discussed.

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Support invoking JS-implemented functions marked as [implicit_jscontext]

I also added tests for [implicit_jscontext] methods with no args/retval and one with just a retval.

Comment 16

[Andrew McCreight (out until 2026) [:mccr8]]

Comment on attachment 8992576 [details] [diff] [review]
Support invoking JS-implemented functions marked as [implicit_jscontext]

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

Thanks, this'll be easier to maintain.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9873d65ac40b
Support invoking JS-implemented XPIDL methods/attributes marked as [implicit_jscontext]. r=mccr8

Comment 18

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/9873d65ac40b