1089230 - add string overload for nsIDOMWindowUtils::loadSheet method (e10s)

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Tomislav Jovanovic :zombie]

running this code in scratchpad of an e10s window:

  let css = 'data:text/css,body{color:red}';
  let uri = Services.io.newURI(css, null, null);

  let tab = gBrowser.addTab('data:text/html,red');
  let mm = tab.linkedBrowser.messageManager;

  mm.addMessageListener(7, ({ target: b }) => {
    let win = b.contentWindow || b.contentWindowAsCPOW;
    let utils = win.getInterface(Ci.nsIDOMWindowUtils);
    utils.loadSheet(uri, null);
  });

  gBrowser.selectedTab = tab;
  mm.loadFrameScript('data:,sendAsyncMessage(7)', true);


results in these errors:

> Error: Permission denied for <moz-nullprincipal:{b31a6f26-a7b8-4c5e-a890-f15f0873e6da}>
> to create wrapper for object of class UnnamedClass
> 
> NS_ERROR_XPC_JS_THREW_NULL: JavaScript component threw a null value as an exception
> 'JavaScript component threw a null value as an exception' when calling method: [nsIURI::scheme]
> 


this used to work, so i tested and found the last working Nightly was from 8/21 (sorry for not reporting earlier, it was hidden behind some other bugs).

so i guess the issue is probably the same or related to "unprivileged junk scope" as described in bug 1062024 comment 9.

Bill or Bobby, can one of you please take a look to: 
1) verify this is what's actually happening, and 
2) if it's something we want to and can fix easily?

Comment 1

[Tomislav Jovanovic :zombie]

this is the last thing not working in sdk/page-mod for e10s..

Comment 2

[Bobby Holley (:bholley)]

I don't understand how this ever would have worked. The code is creating an XPCOM URI object in the parent process, and trying to pass that object to an XPIDL method in the content process. CPOWs only work on the JS level (not the XPCOM level), and either way the only objects we support passing from the parent to the child are callables.

To make this work, you need to create the URI in the same process as |win|, or modify the API so that it also accepts a string (which, as a primitive, can be handled by the CPOW layer). In general, you're not likely to have a huge amount of success using nsIDOMWindowUtils over IPC.

Or is there something I'm missing here?

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, sorry Tom, but we're not going to fix this. I expect this would have worked after we implemented bidirectional CPOWs but before we implemented security wrappers for them. In the future, we'd like to make it harder to pass CPOWs to native code, which will make this even less workable.

Comment 4

[Tomislav Jovanovic :zombie]

to me, this still looks like something a bunch of (non-sdk) addons might be doing.

i'm not asking because i wish to avoid the work: i can easily fix this for page-mod (since it's an async API). the problem is that even SDK exposes this as a sync function, which some addons might be depending on:

https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/stylesheet_utils
https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/stylesheet_style

admittedly, those are low- and mid-level APIs, but still could cause some breakage..


(in any case, i filled bug 1090135, but i still think this might be worth considering)

Comment 5

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #3)
> Yeah, sorry Tom, but we're not going to fix this. I expect this would have
> worked after we implemented bidirectional CPOWs but before we implemented
> security wrappers for them.

Do you mean that we would have created an XPCWrappedJS around the CPOW for the nsIURI?


(In reply to Tomislav Jovanovic [:zombie] from comment #4)
> i'm not asking because i wish to avoid the work: i can easily fix this for
> page-mod (since it's an async API). the problem is that even SDK exposes
> this as a sync function, which some addons might be depending on:

You can still do this synchronously, right? You just need to make sure that the URI gets created in the child process rather than the parent process.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bobby Holley (:bholley) from comment #5)
> Do you mean that we would have created an XPCWrappedJS around the CPOW for
> the nsIURI?

Yes.

I'm going to WONTFIX this. If we run into a lot of addon compat issues in the future, we can re-open.

Comment 7

[Tomislav Jovanovic :zombie]

(In reply to Bobby Holley (:bholley) from comment #5)
> You can still do this synchronously, right? You just need to make sure that
> the URI gets created in the child process rather than the parent process.

i tried using the  contentWindowAsCPOW.URL()  but apparently that doesn't QI to a nsIURI instance acceptable by the loadSheet() method.

the only method (that i know of) of doing something synchronously from the parent process is by 1) getting a hold of some CPOW, and 2) calling a method on it. but that first step also requires an async step (broadcast message from parent, send message from child), so unless i can do something from the nsIDOMWindow CPOW i already have, i'm out of luck.

is there an easier (better) way that i'm not familiar with?

Comment 8

[Bobby Holley (:bholley)]

There's probably some way you could coax your way to a Services object on the content-side. But it would probably be easier to just add a string overload to nsIDOMWindowUtils::LoadStyleSheet that created the URI object in C++.

Comment 9

[Tomislav Jovanovic :zombie]

hey Gabor, it looks like you worked on that method originally (lucky me.. ;)

do you think you could add this string overloaded version?

Comment 10

[Tomislav Jovanovic :zombie]

(re-opening as a different bug, per Bobby's suggestion)

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8515092 [details] [diff] [review]
patch

Does this do what you want?

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8515092 [details] [diff] [review]
patch

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

Assuming this fixes the problem, r=me with those things fixed.

::: dom/base/nsDOMWindowUtils.cpp
@@ +3493,3 @@
>  nsDOMWindowUtils::AddSheet(nsIDOMStyleSheet *aSheet, uint32_t aSheetType)
>  {
>    MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome());

Please duplicate this assert at the top of the new method, for consistency and sanity.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1581,5 @@
>     */
>    void disableDialogs();
>    void enableDialogs();
>    bool areDialogsEnabled();
>  

Need to rev the IID.

@@ +1585,5 @@
>  
>    const unsigned long AGENT_SHEET = 0;
>    const unsigned long USER_SHEET = 1;
>    const unsigned long AUTHOR_SHEET = 2;
> +

Whitespace change?

@@ +1601,5 @@
>  
>    /**
> +   * Same as the above method but allows passing the URI as a string.
> +   */
> +  void loadSheetAsString(in ACString sheetURI, in unsigned long type);

I think "AsString" is kind of a misnomer here. How about loadSheetFromURIString?

Comment 14

[Tomislav Jovanovic :zombie]

Comment on attachment 8515092 [details] [diff] [review]
patch

assuming this ends up callable from the parent, yes.

Comment 15

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/4f1e247709b9

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4f1e247709b9