784686 - Creating websocket from a sandbox without any window object

Status: NEW

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

Description

[Gabor Krizsanits (INACTIVE)]

It would be nice if jetpack sandboxes could create their own websockets.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

I think we want to do this only after bug 775368, since apparently nsIWebSocket will be
removed in that bug and atm the easiest way to fix this would be to add some new
[noscript] to nsIWebSocket.

Comment 2

[Gabor Krizsanits (INACTIVE)]

Dave, I forgot to ask you about the priority of this bug. I know Irakli was quite enthusiastic about this feature...

Comment 3

[Dave Townsend [:mossop]]

P2 in Jetpack terms, lower than the indexed db and the other stylesheet service stuff I think.

Comment 4

[Gabor Krizsanits (INACTIVE)]

So how does websocket work that has been created from a chrome window? What will it send as its origin? And in jetpack where do we want to use this from? I guess from chrome sandboxes (modules) mainly, and I'm a bit concerned that websocket client without any origin info is a bit weird... Adding a constructor to sandbox is really trivial, I'm just not sure if this feature is really useful.

Comment 5

[Gabor Krizsanits (INACTIVE)]

I just had a talk with smaug on irc about this. I could extend the websocket ctor with an origin argument for the chrome case. So the question only what do we want to use this for in jetpack. Irakli? I think you were interested in this feature the most, any thoughts?

Comment 6

[Gabor Krizsanits (INACTIVE)]

After some more talks currently the best approach seem to reuse the sandboxName property that is used to store the location string of the addon. (or if that string is not exactly what we need we can add another string property ofc)

However there is another problem. It turns out that the new dom binding passes the global as nsISupports* global; In case of the global is a sandbox it is just a regular JSObject so it does not implement nsISupports. The global in this case only used to fetch a principal from it by the way. Peter, Boris, Bobby, what do you think about this? Is there a way to specify for the new codegen to pass the global as a regular JSObject*? If not that would mean porting indexeddb to the new bindings will break jetpack support for it.

So the reason why I want to have these constructors in a sandbox, because in jetpack modules are chrome sandboxes. I don't think creating a hidden chrome window for them would be any nicer...

Comment 7

[Gabor Krizsanits (INACTIVE)]

I just realized that I can still just ignore that "global" nsISupports argument and fetch the principal from some place else probably. So the only question is if this is the right thing to do. Exposing DOM API like this in chrome sandboxes without a window/document/real origin.

Comment 8

[Boris Zbarsky [:bzbarsky]]

First of all, I thought sandboxes do in fact have an nsISupports hanging off them.  In particular, if a window was passed to the sandbox constructor, it'll be that window.  If a principal was passed it'll be a PrincipalHolder object.  Either way, it should QI to nsIScriptObjectPrincipal.  Are you not seeing that?  How can I reproduce whatever it is you're seeing?

> Is there a way to specify for the new codegen to pass the global as a regular JSObject*?

That's how it works in Workers, but I don't see that we need that here, especially if all WebSocket wants from what's passed in is a principal...

I think exposing a WebSocket constructor in Chrome sandboxes is just fine.  If we're paranoid, we could do it only when explicitly requested.  Note that I think we might already expose the string encoder/decoder in such cases.

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> it'll be that window.  If a principal was passed it'll be a PrincipalHolder

Sorry, my bad. I have not realized that, thanks for explaining. I was about to get the principal in a more complicated way, but this just works indeed.

There is another issue however... In WebSocket::CreateAndDispatchMessageEvent the JSContext is acquired via nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(GetOwner()) to create some buffers. I think that is really something sandboxes don't have in the PrincipalHolder case (I would be glad if I were wrong again). Is there any context we could use in this case? I'm not sure what is the status around the context stack these days...

http://mxr.mozilla.org/mozilla-central/source/content/base/src/WebSocket.cpp#873

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I think that is really something sandboxes don't have in the PrincipalHolder case

That's correct.  We could maybe fall back to the safe context in cases when we don't have a useful script global there?

Comment 11

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: Fixing WebSocket's compartment usage

The story here is that instead of creating these buffers in random compartment (which is not just not nice, but we happen to catch a system compartment can result in some accessing issues because of the __exposedProps__ stuff), the wrapper is preserved and used as a reference to the right compartment.

Comment 12

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: Adding WebSocket support for sandboxes

bholley: Could you take a look at the construtor part? I ended up adding the origin URI info to the PrincipalHolder. Because I need it sooner in websocket, then the wrapper is attached to the WebSocket instance (from the Constructor), and there is no way to fetch a reference to the compartment private.

smaug: Could you take a look at the removed assertions, and the added mAsiiOrigin member? The later is needed because I can only fetch that info in the Constructor and need to store it somewhere. But since it's there I reused it to calculate it only once in the regular case as well... is that safe/legal to do?

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 683069 [details] [diff] [review]
part1: Fixing WebSocket's compartment usage

Do we really want to preserve all WebSocket objects just to be able to find their scope? It seems like we could just check GetWrapper, and if that fails, do GetParent()->GetCurrentInnerWindow()->FastGetGlobalJSObject().

Comment 14

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #13)
> Comment on attachment 683069 [details] [diff] [review]
> part1: Fixing WebSocket's compartment usage
> 
> Do we really want to preserve all WebSocket objects just to be able to find
> their scope? It seems like we could just check GetWrapper, and if that
> fails, do GetParent()->GetCurrentInnerWindow()->FastGetGlobalJSObject().

Oh, I see. In the sandbox case there is no DOM window. How does the binding stuff work in the sandbox case anyway?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 683069 [details] [diff] [review]
part1: Fixing WebSocket's compartment usage

This is not very pretty, but should work.

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 683072 [details] [diff] [review]
part2: Adding WebSocket support for sandboxes

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3344,5 @@
>              !JS_DefineFunction(cx, sandbox, "XMLHttpRequest", CreateXMLHttpRequest, 0, JSFUN_CONSTRUCTOR))
>              return NS_ERROR_XPC_UNEXPECTED;
> +
> +        if (options.wantWebSocketConstructor &&
> +            !WebSocketBinding::GetProtoObject(cx, sandbox, sandbox))

Hm, what's the deal with this signature here? The GetProtoObject I see in my tree takes two arguments and returns a JSObject. Is this supposed to be installing the proto here?

::: content/base/src/WebSocket.h
@@ +288,5 @@
>    uint32_t mScriptLine;
>    uint64_t mInnerWindowID;
>  
> +  // We calculate the ascii origin only once since the mPrincipal is immutable, 
> +  // and because for the chrome sandbox case we want to calculate it in the 

You've got a bunch of trailing whitespace, here and elsewhere. Can you fix your editor?

::: content/base/src/nsILocationURIHolder.h
@@ +11,5 @@
> +class nsIURI;
> +
> +#define NS_ILOCATIONURIHOLDER_IID \
> +{ 0xfe6f8a8f, 0xfc96, 0x4c07, \
> +{ 0x85, 0x30, 0x72, 0xfc, 0x41, 0x60, 0xb7, 0x6f } };

Any reason to do it this way rather than as an idl file? This seems like more goop.

Comment 17

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #16)
> Hm, what's the deal with this signature here? The GetProtoObject I see in my
> tree takes two arguments and returns a JSObject. Is this supposed to be
> installing the proto here?

Yes. I assume I should update my local copy and the patches... Based on this:
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsXPConnect.cpp#1164
I'm going to use GetConstructorObject here.


> You've got a bunch of trailing whitespace, here and elsewhere. Can you fix
> your editor?

This is getting really annoying. I did that once, and the result was that I committed a patch that fixed white spaces in the whole file and I was asked not to do that in the future... But I will look into this to find some solution for it. At some point I'm going to write a script for formatting for sure...

> Any reason to do it this way rather than as an idl file? This seems like
> more goop.

No. I can do that I guess...

Comment 18

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #17)
> > You've got a bunch of trailing whitespace, here and elsewhere. Can you fix
> > your editor?
> 
> This is getting really annoying. I did that once, and the result was that I
> committed a patch that fixed white spaces in the whole file and I was asked
> not to do that in the future... But I will look into this to find some
> solution for it. At some point I'm going to write a script for formatting
> for sure...

I'd suggest configuring your editor to make trailing whitespace visible (I have it set to show a blue '.' in vim). This lets your notice when the lines you add have trailing whitespace, but doesn't automatically fix everything else in the file.

Comment 19

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #16)
> Any reason to do it this way rather than as an idl file? This seems like
> more goop.

So actually there was. Because of symmetry. nsIScriptObjectPrincipal is implemented like that, and it's getter method nsIPrincipal* GetPrincipal() does not have an idl like signature. So A., I do the same thing for the other getter like I did, or B., I change nsIScriptObjectPrincipal too, create an idl for it and change every instance of GetPrincipal all over the place. (I have no clue how much work that would be tbh). I leave the decision up to you.

Comment 20

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)
> (In reply to Bobby Holley (:bholley) from comment #16)
> > Any reason to do it this way rather than as an idl file? This seems like
> > more goop.
> 
> So actually there was. Because of symmetry. nsIScriptObjectPrincipal is
> implemented like that, and it's getter method nsIPrincipal* GetPrincipal()
> does not have an idl like signature. So A., I do the same thing for the
> other getter like I did, or B., I change nsIScriptObjectPrincipal too,
> create an idl for it and change every instance of GetPrincipal all over the
> place. (I have no clue how much work that would be tbh). I leave the
> decision up to you.

Certainly don't do that. But you can just mark the method [notxpcom] in IDL and use whatever kind of signature you want.

Comment 21

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: Fixing WebSocket's compartment usage

I've added the test you requested about attaching listener from a different compartment.

Comment 22

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: Adding WebSocket support for sandboxes

Updated version.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 685108 [details] [diff] [review]
part1: Fixing WebSocket's compartment usage

># HG changeset patch
># Parent bc69705c162dad69a4c6b42aedbd10a75d7d1a77
># User Gabor Krizsanits <[e-mail]>
>Bug 784686 - part1: Fixing WebSocket's compartment usage
>
>diff --git a/content/base/src/WebSocket.cpp b/content/base/src/WebSocket.cpp
>--- a/content/base/src/WebSocket.cpp
>+++ b/content/base/src/WebSocket.cpp
>@@ -476,17 +476,23 @@ WebSocket::~WebSocket()
>     Disconnect();
>   }
>   nsLayoutStatics::Release();
> }
> 
> JSObject*
> WebSocket::WrapObject(JSContext* cx, JSObject* scope, bool* triedToWrap)
> {
>-  return WebSocketBinding::Wrap(cx, scope, this, triedToWrap);
>+  // Preserving wrapper for being able to get the right compartment from it later.
>+  // (see buffer object creation in CreateAndDispatchMessageEvent)
>+  JSObject* wrapper = WebSocketBinding::Wrap(cx, scope, this, triedToWrap);
>+  nsContentUtils::PreserveWrapper(
>+    static_cast<nsIDOMEventTarget*>(
>+      static_cast<nsDOMEventTargetHelper*>(this)), this);
>+  return wrapper; 
> }
  // ... comment goes here...
  nsContentUtils::PreserveWrapper(
    static_cast<nsIDOMEventTarget*>(
      static_cast<nsDOMEventTargetHelper*>(this)), this);
  return WebSocketBinding::Wrap(cx, scope, this, triedToWrap);


>+function testWebSocket () {
>+  var frame1 = document.getElementById("frame1");
>+  frame1.onload = function() {
>+    var ws = frame1.contentWindow.ws;
>+    ws.onopen = function(e) {
>+      ws.send("data");
>+    }
This is error prone. It is possible that open event has fired already.
You need to create ws object (synchronously) right before setting load event listener.

I'd like to see the test fixed.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 685110 [details] [diff] [review]
part2: Adding WebSocket support for sandboxes

>   nsRefPtr<WebSocket> webSocket = new WebSocket();
>+  if (nsContentUtils::IsSystemPrincipal(principal)) {
>+    nsCOMPtr<nsILocationURIHolder> holder = do_QueryInterface(aGlobal);
>+    if (!holder) {
>+      aRv.Throw(NS_ERROR_FAILURE);
>+      return nullptr;
>+    }

So doesn't this change the behavior if principal is system principal, but aGlobal
isn't nsILocationURIHolder, but nsPIDOMWindow? I don't think we want that.


Could you fix that and add tests for chrome initiating websocket?
(I know Origin handling is odd)

Comment 25

[Gabor Krizsanits (INACTIVE)]

So it seems to be crashing on try during GC... Could not reproduce locally, there are a few stack traces to be found here: https://tbpl.mozilla.org/?tree=Try&rev=cda05ce42149 an example: https://tbpl.mozilla.org/php/getParsedLog.php?id=17372417&tree=Try&full=1#error0

I guess the preserve wrapper thing has some side effects combined with the cycle collector code of websocket... I'm not familiar enough with this cycle collector code, tomorrow I will look into what NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN really does here... But if by looking at it someone has a clue what could go wrong here I'm happy to hear any suggestions.

(and please ignore the failing oth that's because of dumb mistake in test_websocket_chrome.xul I made)

Comment 26

[Gabor Krizsanits (INACTIVE)]

After the WebSocket instance is destructed it has not been removed from the XPCJSRuntime's mJSHolders map. So when xpc_UnmarkSkippableJSHolders is called from the CC it tries to call CanSkip with an invalid holder pointer.
I've been reading code around the life time of an XPCWrappedNative for a while, but I don't see any guard against this in WrappedNativeFinalize. Shouldn't we make sure that we remove the related holder from the mJSHolders map?

Comment 27

[Andrew McCreight [:mccr8]]

I'm not sure of the particulars of web sockets, but in general if you add a call to HoldWrapper, you need to add a call to ReleaseWrapper. That will remove it from the holders maps. That needs to be done in UNLINK, I think.

Comment 28

[Gabor Krizsanits (INACTIVE)]

(In reply to Andrew McCreight [:mccr8] from comment #27)
> I'm not sure of the particulars of web sockets, but in general if you add a
> call to HoldWrapper, you need to add a call to ReleaseWrapper. That will
> remove it from the holders maps. That needs to be done in UNLINK, I think.

Maybe I'm missing something but if I get it correctly that is already done by this macro:
NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(WebSocket,
                                                nsDOMEventTargetHelper)

which calls release wrapper here:
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsDOMEventTargetHelper.cpp#24

Comment 29

[Andrew McCreight [:mccr8]]

Oh, right! nsDOMEventTargetHelper even also explicitly calls ReleaseWrapper in its destructor, so it is weird that you are seeing this problem.

Comment 30

[Gabor Krizsanits (INACTIVE)]

Both weird and scary tbh... This happens btw after a nuke window call probably, since I got plenty of dead object proxy error on the console before it. But what really matters that this should be guarder at XPCJSRuntime level imo. Because it means that it destroys objects while it still stores a raw pointer to it in its own map...

Comment 31

[Edouard Oger [:eoger]]

In bug 1490671 we are creating a windowless browser to use WebSockets from chrome code. See [0].
This is somewhat fine as this code will be loaded at most probably once or twice during a "Firefox installation lifetime", I just wanted to let you know it would still be useful to fix this bug :)

[0] https://github.com/mozilla/fxa-pairing-channel/blob/35356f339099894136e452884287276cc6fda716/dist/FxAccountsPairingChannel.js#L11-L13

Comment 32

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.