Closed Bug 1089230 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: XPConnect, defect)

defect
Not set
normal
Points:
1

Tracking

()

RESOLVED FIXED
mozilla36
Iteration:
36.2
Tracking Status
e10s m4+ ---

People

(Reporter: zombie, Assigned: mossop)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file)

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?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(bobbyholley)
this is the last thing not working in sdk/page-mod for e10s..
Blocks: 1066685
tracking-e10s: --- → ?
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?
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(wmccloskey)
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)
(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.
(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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
(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?
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++.
hey Gabor, it looks like you worked on that method originally (lucky me.. ;)

do you think you could add this string overloaded version?
Flags: needinfo?(gkrizsanits)
(re-opening as a different bug, per Bobby's suggestion)
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: nsIURI instances don't CPOW (or nsIDOMWindowUtils.loadSheet doesn't work in e10s) → add string overload for nsIDOMWindowUtils::loadSheet method (e10s)
Attached patch patchSplinter Review
Assignee: nobody → dtownsend+bugmail
Status: REOPENED → ASSIGNED
Flags: needinfo?(gkrizsanits)
Attachment #8515092 - Flags: review?(bobbyholley)
Comment on attachment 8515092 [details] [diff] [review]
patch

Does this do what you want?
Attachment #8515092 - Flags: review?(bobbyholley) → feedback?(tomica+amo)
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?
Attachment #8515092 - Flags: review+
Comment on attachment 8515092 [details] [diff] [review]
patch

assuming this ends up callable from the parent, yes.
Attachment #8515092 - Flags: feedback?(tomica+amo) → feedback+
Blocks: 1090135
https://hg.mozilla.org/integration/fx-team/rev/4f1e247709b9
Iteration: --- → 36.2
Points: --- → 1
Flags: qe-verify-
Flags: firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/4f1e247709b9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1093048
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.