Status

()

enhancement
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: wfernandom2004, Assigned: baku)

Tracking

(Blocks 1 bug, {dev-doc-needed})

unspecified
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 disabled, firefox36 disabled, firefox37 disabled, firefox38 fixed, relnote-firefox 38+)

Details

(Whiteboard: [games:p1])

Attachments

(5 attachments, 33 obsolete attachments)

23.33 KB, patch
Details | Diff | Splinter Review
53.88 KB, patch
Details | Diff | Splinter Review
73.19 KB, patch
smaug
: review+
Details | Diff | Splinter Review
25.28 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.98 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; pt-BR; rv:1.9.0.11) Gecko/2009060309 Ubuntu/8.04 (hardy) Firefox/3.0.11
Build Identifier: 

Make possible for workers to create websockets.

Reproducible: Always
(Reporter)

Updated

10 years ago
Depends on: websocket
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 1

9 years ago
Confirming bug still exists in Firefox 4 Beta 1.
(Reporter)

Comment 2

9 years ago
I will start to work on this next week.
(Reporter)

Updated

9 years ago
Assignee: nobody → wfernandom2004
Status: NEW → ASSIGNED

Comment 3

9 years ago
Thank you, just a note that this actually had a decent performance improvement for me on Chrome...

So its a very worthwhile implementation.

I put my WebSocket client on a WebWorker thread to process the messages and send the data back with postMessage, and it helped a good bit.

I'm guessing the downloading and uploading of data can be CPU consuming and slows down rendering on main thread.
(Reporter)

Comment 4

9 years ago
Sorry, I've started this work, however I'm a lot busy right now in my new work. Can anyone else implement this bug, please?
Status: ASSIGNED → NEW
(Reporter)

Updated

9 years ago
Assignee: wfernandom2004 → nobody
changing category to networking:websockets - its really the content side not networking, but its a better match than 'general'.
Component: General → Networking: WebSockets
QA Contact: general → networking.websockets
This *just* needs someone to write an implementation using JSAPI directly, the same way we've done with all other APIs that we have in workers.

And also to figure out how to access websocket channels off the main thread.

This is certainly a lot of work though. And the first part (i.e. everything by the websocket channels part) will get a lot easier once we have the new DOM Bindings.

Comment 8

7 years ago
Jonas,

Is there anyone you can suggest to work on this?  It sounds like it's mostly DOM work not necko, module the off-main-thread websocket channel issue. (Can a websocket channel be accessed simultaneously by the main thread and the worker thread?  If not, it's probably not so much work, I'd guess.)
No, I don't have anyone in mind.

A websocket object will only ever be accessed on a single thread for the lifetime of the object.
Bug still in 12, even though WebSocket is no longer prefixed and it's part of the WebSocket spec:

From http://www.w3.org/TR/websockets/#the-websocket-interface:

> This constructor must be visible when the script's global object is either
> a Window object or an object implementing the WorkerUtils interface.

It's causing a major problem for us, because our library will have to special case on firefox and not use a webworker when we want to do our connection to the remote server.

This page allows you to check to see if WebSocket is available inside a WebWorker.
(Assignee)

Updated

7 years ago
Assignee: nobody → amarchesini
(Assignee)

Comment 12

7 years ago
I'm just using bugzilla as a backup of my code.
I hope to have something ready for the next week.
(Assignee)

Updated

7 years ago
Depends on: 775368
No longer depends on: websocket
(Assignee)

Updated

7 years ago
Attachment #646456 - Attachment description: still working in progress - do not review! → still work in progress - do not review!
(Assignee)

Comment 13

7 years ago
Compared with the previous implementation this patch does:

1. WebSocketHelper - a virtual class that implements the "logic" of the WebSocket. It can run on any thread. It needs a WebSocketProxy in order to comunicate to/from the nsWebSocketChannel/nsWebSocketListener.

2. WebSocketProxy - a virtual class that is needed by a WebSocketHelper.

3. WebSocketImpl - It inherits the WebSocketProxy - it runs on the main thread ONLY and it proxies any communication from nsWebSocketChannel/nsWebSocketListener to the WebSocketHelper.

4. WebSocketWorkerImpl - It inherits the WebSocketProxy. It runs on a worker thread. It creates+proxies a WebSocketImpl running on the main thread.

5. mozilla::dom::WebSocket - it inherits the WebSocketHelper and this is the object for the main thread. It uses the WebSocketImpl - everything runs in the main thread.

6. mozilla::dom::Workers::WebSocket - it inherits the WebSocketHelper and it's the object for the worker thread. It uses the WebSocketWorkerImpl.
Attachment #646456 - Attachment is obsolete: true
Attachment #648650 - Flags: review?(jonas)
Attachment #648650 - Flags: review?(bent.mozilla)
Attachment #648650 - Flags: review?(Ms2ger)
(Assignee)

Comment 14

7 years ago
unfortunately there are some UTF-8 non-characters in a test.js file...

Comment 15

7 years ago
amarchesini,

So this patch can't be viewed in bugzilla splinter, nor does it apply cleanly to the tree (you seem to be assuming that the existing code in /content/base/src is called "WebSocket.h|cpp", when the files are called "nsWebSocket.h|cpp", for one thing).   

Please fix and upload a patch that at least applies to the tree.  I'd also like to be a reviewer here: you definitely need either me or :smaug to +r the patch, in addition to someone who knows Workers (I assume bent or sicking).   Keeping it to 2 reviewers for starters is probably enough.

Looking forward to seeing what you've done!
I think what you need to do is to enable "git style patches" in mercurial. Add the following to your .hgrc

[diff]
git = 1

Comment 17

7 years ago
> you definitely need either me or :smaug to +r the patch

To be precise: I can't give you the final +r to the patch (not a DOM peer: smaug is the peer who's been doing +r for websockets), but I'm happy to take a first look at it since I've been working on websockets stuff more than anyone else recently.
(In reply to Jason Duell (:jduell) from comment #15)
> amarchesini,
> 
> So this patch can't be viewed in bugzilla splinter, nor does it apply
> cleanly to the tree (you seem to be assuming that the existing code in
> /content/base/src is called "WebSocket.h|cpp", when the files are called
> "nsWebSocket.h|cpp", for one thing).   
> 
> Please fix and upload a patch that at least applies to the tree.

Jason, this patch applies over the patch in the dependent bug (bug 775368), as is customary.
(Assignee)

Comment 19

7 years ago
Attachment #648650 - Attachment is obsolete: true
Attachment #648650 - Flags: review?(jonas)
Attachment #648650 - Flags: review?(bent.mozilla)
Attachment #648650 - Flags: review?(Ms2ger)
Attachment #649111 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #649111 - Attachment is patch: true
Attachment #649111 - Flags: review?(jduell.mcbugs)
Attachment #649111 - Flags: review?(bent.mozilla)
Attachment #649111 - Flags: review?
(Assignee)

Comment 20

7 years ago
Any news about this?
Hey, sorry, all my time is being used for b2g stuff at the moment. I'll get to this as soon as we clear our next deadline.

Comment 22

7 years ago
same here.  Very excited about this work, but slammed with B2G until the 27th.
(Assignee)

Comment 23

7 years ago
Rebased + some bug fixed.
Attachment #649111 - Attachment is obsolete: true
Attachment #649111 - Flags: review?(jduell.mcbugs)
Attachment #649111 - Flags: review?(bent.mozilla)
Attachment #654806 - Flags: review?(jduell.mcbugs)
Attachment #654806 - Flags: review?(bent.mozilla)
(Assignee)

Comment 24

7 years ago
Posted patch patch 2 (obsolete) — Splinter Review
rebased on 785319
Attachment #654806 - Attachment is obsolete: true
Attachment #654806 - Flags: review?(jduell.mcbugs)
Attachment #654806 - Flags: review?(bent.mozilla)
Attachment #655925 - Flags: review?(jduell.mcbugs)
Attachment #655925 - Flags: review?(bent.mozilla)
(Assignee)

Updated

7 years ago
Whiteboard: [need review]
(Assignee)

Comment 25

7 years ago
Posted patch patch 3 (obsolete) — Splinter Review
Support for preferences.
Attachment #655925 - Attachment is obsolete: true
Attachment #655925 - Flags: review?(jduell.mcbugs)
Attachment #655925 - Flags: review?(bent.mozilla)
Attachment #658515 - Flags: review?(jduell.mcbugs)
Attachment #658515 - Flags: review?(bent.mozilla)
Comment on attachment 658515 [details] [diff] [review]
patch 3

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

You need some comments explaining how this all works.  Figuring out what all these different classes do is very hard.  There are proxies that inherit from impls and impls that inherit from proxies and who knows what else going on.

::: content/base/src/WebSocketImpl.cpp
@@ +290,5 @@
> +// C++ traverse
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(WebSocketImpl)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mChannel)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mURI)
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

I don't think this is necessary.  Channels and URIs should not be cycle collected.

@@ +342,5 @@
> +
> +  nsCOMPtr<nsILoadGroup> loadGroup;
> +  GetLoadGroup(getter_AddRefs(loadGroup));
> +  if (loadGroup)
> +    loadGroup->RemoveRequest(this, nullptr, NS_OK);

if (foo) {
  bar;
}

::: dom/workers/WebSocketWorker.cpp
@@ +140,5 @@
> +  mWorkerPrivate->AssertIsOnWorkerThread();
> +  MOZ_COUNT_CTOR(mozilla::dom::workers::WebSocket);
> +
> +  // Creation of the proxy:
> +  SetProxy(new WebSocketWorkerImpl(this));

I think you should probably drop the WebSocketWorkerImpl and just create the proxy on the main thread.

::: dom/workers/WebSocketWorkerImpl.cpp
@@ +281,5 @@
> +WebSocketWorkerImpl::WebSocketWorkerImpl(WebSocket* aWebSocket)
> +: WebSocketProxy(aWebSocket),
> +  mWebSocket(aWebSocket)
> +{
> +  MOZ_COUNT_CTOR(mozilla::dom::workers::WebSocketImpl);

This doesn't match the class name, which is going to be very confusing when someone leaks these.

::: dom/workers/WebSocketWorkerImpl.h
@@ +29,5 @@
> +  {
> +     MOZ_COUNT_DTOR(mozilla::dom::workers::WebSocketWorkerProxy);
> +  }
> +
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WebSocketWorkerProxy)

This class inherits from WebSocketImpl, which has code in its ctor/dtor to abort if it's constructed or destroyed off the main thread, but it has threadsafe refcounting?  That doesn't make any sense.

@@ +101,5 @@
> +
> +public:
> +  WorkerPrivate* GetWorkerPrivate() { return mWebSocket->GetWorkerPrivate(); }
> +
> +public:

public:

  foo

public:

  bar

Please drop the second 'public'.
Attachment #658515 - Flags: review?(bent.mozilla) → review-
What is happening with this bug?
(Assignee)

Comment 28

7 years ago
I have to spend time on this bug mostly writing documentation. I think the best approach can be to review this code face to face with someone because the patch is huge, it changes too much code etc.

Comment 29

6 years ago
Jagex has released a Runescape Bestiary: http://services.runescape.com/m=news/try-the-runescape-bestiary. At this moment though, the actual bestiary only shows a big Google Chrome logo and a request to "upgrade" my browser to the latest Google Chrome, as it apparently is the only browser which "fully" supports HTML5 and WebGL.

I first wanted to file a tech evangelism bug, but on the forum, Mod Chrisso mentions this bug is the cause of not having it available in Firefox (source: http://services.runescape.com/m=forum/l=0/sl=0/forums.ws?15,16,839,64246943,goto,8).

I don't know if this comment adds anything to the priority of this bug, but I guess it doesn't hurt to mention it :)

Comment 30

6 years ago
Comment on attachment 658515 [details] [diff] [review]
patch 3

I'm going to assume for now that khuey's r- means I can wait for a new patch before reviewing.  I also assume this bug will become more lively when B2G cools off a bit.
Attachment #658515 - Flags: review?(jduell.mcbugs)
Attachment #627152 - Attachment mime type: text/plain → text/html
This is required by the Runescape Bestiary app[1], if you bypass the userAgent (contains 'chrome') and appVersion (contains 'Chrome/N.', N > 21) sniffing that redirects to a 'download chrome' overlay. All the other HTML5 tech seems to be in place except for WebSockets in Workers.
Vlad and I agree that we should priorities this as a result of the attention it's likely to get.  Runescape is solid brand and has a US Alexa rank of 1,446.  I'm putting this up to a P1 for games as a result and let's try and see if we can get them to clear that error under Firefox.  It would be good to have this working by GDC (end of March).
Whiteboard: [need review] → [need review][games:p1]

Comment 33

6 years ago
I've been on the CC list for this issue for a while - if there's anything we (Jagex) can do, please do get in touch with me directly. Of course as soon as a patch hits release builds, we'll remove the Chrome only guard on the Bestiary and let our player base know.

We have internal release targets for the full RuneScape game on this platform in the next couple of months and of course we'd love for it to run on FireFox at launch.

Many thanks.
We're in the process of totally reworking our memory model and event systems for workers in order to do things like this.  End of March is probably too optimistic for that.
Thanks for the Kyle for the info on time frame.  I'll reach out to Chris and see if there are some alternative solution possible in the short term.

Updated

6 years ago
Depends on: 845545, 853893
Is there an updated on progress?
Blocks: 898706
Duplicate of this bug: 896196
I don't think Andrea is working on this right now. We're also going to have to have a dramatically different approach here once bug 853893 is done since we'll actually be able to mostly use the main-thread implementation on the worker thread.
Assignee: amarchesini → nobody
All blocking bugs has been fixed! Who will deal with this bug?

Comment 41

6 years ago
I think most of this work is more DOM-y than necko-y, i.e. I can't think of anyone in the networking team who knows the worker-related issues well enough to work on this. Doug, any thoughts on who could take this?
Component: Networking: WebSockets → DOM: Workers
Flags: needinfo?(doug.turner)
Jason: The one piece that we could need help with is the ability to use a websocket channel directly from the worker thread. We can always proxy through the main thread, but the less we need to, the better.

Comment 43

6 years ago
From a brief look over the WebSocketChannel code, it looks like it's fine to send() messages from non-main threads. Also ok to close() off-main.  We don't currently have a method to deliver incoming messages to a non-main thread, but we could add one. AsyncOpen() is trickier--we currently use the Prefs service, and also do have a global admissionsService object that we might need to make thread-safe.  We'd also need to make sure we don't run afoul of nsIURI thread usage constraints (from talking to bsmedberg it sounds like we need to create URIs on the main thread but can use them on other threads as long as we don't modify them concurrently from multiple threads--but I got a sense from sicking that nsIURI usage is more limited?). My gut sense is that if we can live with main thread asyncOpen but have off-main send/recv we're good enough.

I'm not 100% sure about some of the accessor functions: uri, baseUri, loadGroup, notificationCallbacks, etc. The referred-to objects would need to have threadsafe addref/release at a minimum.  URIs don't.  LoadGroup/callbacks generally do.

Do you want to create websocketChannel objects off-main thread, or can that be main thread-only too?

Let me know if you want off-main websocket msg delivery and I'll open a bug for it.
Flags: needinfo?(doug.turner) → needinfo?(jonas)

Updated

6 years ago
Whiteboard: [need review][games:p1] → [games:p1]

Comment 44

6 years ago
(In reply to Jason Duell (:jduell) from comment #43)
> Do you want to create websocketChannel objects off-main thread, or can that
> be main thread-only too?
> 
> Let me know if you want off-main websocket msg delivery and I'll open a bug
> for it.

I would like to be able to create the WebSockets directly from inside the WebWorker (not on the main thread). In my own use case, I'm developing a communications library that this way it could be loaded and run on a background worker...

Comment 45

6 years ago
We can hopefully allow JS code to create a "Websocket" (which is a DOM object) on a worker thread, even if the implementation winds up proxing the actual websocketchannel creation/asyncOpen to the main thread.

Comment 46

6 years ago
(In reply to Jason Duell (:jduell) from comment #45)
> We can hopefully allow JS code to create a "Websocket" (which is a DOM
> object) on a worker thread, even if the implementation winds up proxing the
> actual websocketchannel creation/asyncOpen to the main thread.

Could this be also done with WebRTC PeerConnection/DataChannels?

Comment 47

6 years ago
> Could this be also done with WebRTC PeerConnection/DataChannels?

I don't know, but maybe jesup does...
Flags: needinfo?(rjesup)
(In reply to Jason Duell (:jduell) from comment #43)
> From a brief look over the WebSocketChannel code, it looks like it's fine to
> send() messages from non-main threads. Also ok to close() off-main.

Is that true in child processes too?

> We
> don't currently have a method to deliver incoming messages to a non-main
> thread, but we could add one.

That would be awesome! Though see above about child processes.

> AsyncOpen() is trickier

That's fine. In general, the more callsites that we make available off-main-threads, the less code we have to fork between main-thread/off-main-thread. It's not an all-or-nothing.

As long as we document in the interface which parts can be used off the main thread, and which ones can't.

> I'm not 100% sure about some of the accessor functions: uri, baseUri,
> loadGroup, notificationCallbacks, etc. The referred-to objects would need to
> have threadsafe addref/release at a minimum.  URIs don't. 
> LoadGroup/callbacks generally do.

Could we avoid calling these from the API implementation then?

> Do you want to create websocketChannel objects off-main thread, or can that
> be main thread-only too?

Creating the channel on the main thread, but doing as much else as we can off the main thread, sounds like a good compromise.

> Let me know if you want off-main websocket msg delivery and I'll open a bug
> for it.

Please do.


Please, lets keep other APIs to other bugs. This bug is about WebSocket and WebSocket only. And we do know that plenty of people want it, that's not at question.

Updated

6 years ago
Depends on: 925623

Comment 49

6 years ago
(In reply to Jason Duell (:jduell) from comment #47)
> > Could this be also done with WebRTC PeerConnection/DataChannels?
> 
> I don't know, but maybe jesup does...

On https://bugzilla.mozilla.org/show_bug.cgi?id=922363 he was the one suggesting me that maybe some of the work done for WebSockets could be re-used for DataChannels... :-)

Comment 50

6 years ago
(In reply to Jason Duell (:jduell) from comment #47)
> > Could this be also done with WebRTC PeerConnection/DataChannels?
> 
> I don't know, but maybe jesup does...

On https://bugzilla.mozilla.org/show_bug.cgi?id=922363 he was the one suggesting me that maybe some of the work done for WebSockets could be re-used for DataChannels... :-)

Comment 51

6 years ago
e10s is hard to optimize here--we can send/rev IPDL messages to non-main threads, but *all* msgs have to go to those thread.  In this case we'd ideally like the socket transport thread to talk IPDL to the worker thread in the child.  We might be able to receive the initial IPDL channel constructor on the socket transport, and have it just dispatch the actual construction to the main thread.  I'll see what I can do.

> Could we avoid calling these from the API implementation then?

If you mean avoid calling them internally from the other API functions (open, send, close, etc), I think the answer is yes.

Filed bug 925623 for the necko off-main thread work here.

(clearing needsinfo for :jesup.  piranna, feel free to email/ping him elsewhere on the datachannels stuff.)
Flags: needinfo?(rjesup)
Flags: needinfo?(jonas)

Comment 52

6 years ago
(In reply to Jason Duell (:jduell) from comment #51)
> (clearing needsinfo for :jesup.  piranna, feel free to email/ping him
> elsewhere on the datachannels stuff.)

Ok, thanks for the advice.

Comment 53

5 years ago
what is the current status of this bug?
is somebody working on it or does it need other bugs to be fixed first?

Comment 54

5 years ago
What still needs to be done to get this bug fixed?
(Assignee)

Comment 55

5 years ago
I'm working on it. The main problem is the event management in workers. But it should be too complex. Maybe next week I submit a patch for a review.
(Assignee)

Comment 56

5 years ago
Posted patch patch 1 - WebSocketImpl (obsolete) — Splinter Review
This first patch splits DOMEventTargetHelper from the rest of the interfaces.
The reason of this is because DOMEventTargetHelper is not thread-safe.
Attachment #627152 - Attachment is obsolete: true
Attachment #658515 - Attachment is obsolete: true
By "not threadsafe", do you mean that you can't use it in worker threads, or that you can't use it from multiple threads at the same time?

It should be possible to use DOMEventTargetHelper from worker threads. However you indeed won't be able to then addref it from the main thread.

But I'd rather try to avoid calling addref/release from the main thread than to try to avoid using DOMEventTargetHelper. You will have to use cycle collection of the WebSocket object anyway, and that will make it impossible to call addref/release from the main thread.

What we do in other APIs is to addref on the worker thread, then send some message to the main thread so that work can be done there, then send a message back to the worker thread in order to do the release.

Would that be possible here too?
(Assignee)

Comment 58

5 years ago
> By "not threadsafe", do you mean that you can't use it in worker threads, or
> that you can't use it from multiple threads at the same time?

The second one. For how the WebSocketChannel works, I have to call a couple of methods from the main-thread. But I cannot move a DOMEventTargetHelper from a worker thread to the main thread because it's not thread-safe.

So what this patch does is to split DOMEventTargetHelper and the rest. The rest is a WebSocketImpl that can be moved to any thread and it talks with its WebSocket DOMEventTargetHelper object only on the right thread.

> Would that be possible here too?

Sure :) this is what I'm doing.
(Assignee)

Comment 59

5 years ago
Posted patch patch 1 - WebSocketImpl (obsolete) — Splinter Review
Attachment #8406170 - Attachment is obsolete: true
Attachment #8410177 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 60

5 years ago
This second patch has 2 tests: a basic test and an advanced one. The basic test is a copy of content/base/test/test_websocket_basic.html and it passes.

The second test is a copy of content/base/test/test_websocket.html but it still have problems in WebSocketChannel object. I'll discuss these issues by emails with Steve and jduell.
(Assignee)

Comment 61

5 years ago
Attachment #8410180 - Attachment is obsolete: true
Attachment #8410842 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 62

5 years ago
The only remaining issue is about test44 where it crashes because of this:

js/xpconnect/src/xpcprivate.h:273:

    // These get non-addref'd pointers
    static nsXPConnect*  XPConnect()
    {
        // Do a release-mode assert that we're not doing anything significant in
        // XPConnect off the main thread. If you're an extension developer hitting
        // this, you need to change your code. See bug 716167.
        if (!MOZ_LIKELY(NS_IsMainThread()))
            MOZ_CRASH();

Unwrapping the param of send():

#3  0x00007fffef979942 in mozilla::dom::UnwrapArg<nsIDOMBlob, nsIDOMBlob> (cx=0x7fff280295d0, v=..., ppArg=0x7fff327fc430, ppArgRef=0x7fff327fc400, vp=...) at ../../dist/include/mozilla/dom/BindingUtils.h:65
#4  0x00007fffeff8ef18 in mozilla::dom::WebSocketBinding::send (cx=0x7fff280295d0, obj=..., self=0x7fff280d3e60, args=...) at ../build/dom/bindings/WebSocketBinding.cpp:538
You can't use the main-thread Blob binding on workers, of course.

Sounds like you need separate worker and non-worker bindings for WebSocket, or to convert Blob to WebIDL.
(In reply to Boris Zbarsky [:bz] from comment #63)
> or to convert Blob to WebIDL.

... which is bug 827823 for those playing along at home.
(Assignee)

Comment 65

5 years ago
(In reply to Andrew Overholt [:overholt] from comment #64)
> (In reply to Boris Zbarsky [:bz] from comment #63)
> > or to convert Blob to WebIDL.
> 
> ... which is bug 827823 for those playing along at home.

I don't know if that bug is going to land soon. Maybe we can have a temporary solution?
What about if we change the webidl file in:

  void send(any data);

Then, when the blob is ported to WebIDL, we can re-enable the current webidl. How does it sound?
(Assignee)

Updated

5 years ago
Flags: needinfo?(bzbarsky)
"Sounds like you need separate worker and non-worker bindings for WebSocket".  The the codegen will just "deal", I would think (by passing blobs to your worker binding as JSObject* and letting you handle the case when it's not actually a blob).
Flags: needinfo?(bzbarsky)
Yeah, using 'any' sounds ok for now. That way we can use the same code in main thread and in workers which is a big win.
(Assignee)

Comment 69

5 years ago
Attachment #8410842 - Attachment is obsolete: true
Attachment #8410842 - Flags: review?(jduell.mcbugs)
Attachment #8411685 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 70

5 years ago
With this custom send() method WebSockets are fully working on workers.
I'm going to push these patches to try and I'll post the result here.

jduell, I ask you to review this code, but maybe you want a feedback from bent for workers and bz for the JS api/WebIDL.
Attachment #8411687 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 71

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=eaab57a27f62

It's not fully green. The issues are related to WebSocketChannel, probably. I'm debugging it.

Comment 72

5 years ago
Comment on attachment 8410177 [details] [diff] [review]
patch 1 - WebSocketImpl

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

Looks good to me overall.  +r on the necko bits, but you need a DOM/content peer for the rest.  smaug has done the previous websocket reviews FWIW.
Attachment #8410177 - Flags: review?(jduell.mcbugs)
Attachment #8410177 - Flags: review?(bugs)
Attachment #8410177 - Flags: review+

Updated

5 years ago
Attachment #8411685 - Flags: review?(jduell.mcbugs)
Attachment #8411685 - Flags: review?(bugs)
Attachment #8411685 - Flags: review+

Comment 73

5 years ago
Comment on attachment 8411687 [details] [diff] [review]
patch 3 - Custom WebSocket::Send() for workers

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

No necko bits in this one.
Attachment #8411687 - Flags: review?(jduell.mcbugs) → review?(bugs)
(Assignee)

Comment 74

5 years ago
Posted patch patch 1 - WebSocketImpl (obsolete) — Splinter Review
Attachment #8410177 - Attachment is obsolete: true
Attachment #8410177 - Flags: review?(bugs)
Attachment #8417944 - Flags: review?(bugs)
(Assignee)

Comment 75

5 years ago
Posted patch patch 1 - WebSocketImpl (obsolete) — Splinter Review
Sorry, the first patch was not up-to-date.
Attachment #8417944 - Attachment is obsolete: true
Attachment #8417944 - Flags: review?(bugs)
Attachment #8417945 - Flags: review?(bugs)
(Assignee)

Comment 76

5 years ago
Attachment #8411685 - Attachment is obsolete: true
Attachment #8411687 - Attachment is obsolete: true
Attachment #8411685 - Flags: review?(bugs)
Attachment #8411687 - Flags: review?(bugs)
Attachment #8417946 - Flags: review?(bugs)
Comment on attachment 8417945 [details] [diff] [review]
patch 1 - WebSocketImpl

I tried to apply this to m-c and to inbound and then tried some older
changeset as parent too, but no luck.
Is this stuff on top of some other patches?
Comment on attachment 8417945 [details] [diff] [review]
patch 1 - WebSocketImpl

>   nsRefPtr<WebSocket> webSocket = new WebSocket(ownerWindow);
>-  nsresult rv = webSocket->Init(aGlobal.GetContext(), principal,
>-                                aUrl, protocolArray);
>+  nsresult rv = webSocket->mImpl->Init(aGlobal.GetContext(), principal,
>+                                       aUrl, protocolArray);
In this kind of setup Init() shouldn't keep mImpl alive, but the caller.
So create first websocket, then create Impl and assign it to local nsRefPtr and then
call Init. Something like that.


But hard to debug the issue without knowing how to build this stuff.
Based on the assertions something is going wrong with threads.
Assertion failure: mTargetThread, at /builds/slave/try-lx-00000000000000000000000/build/netwerk/ipc/ChannelEventQueue.cpp:74 certainly doesn't look good.
Though, that assertion seems to be about the second patch.
Could you push to try without the second patch, and perhaps tell how to build the first patch.
Attachment #8417945 - Flags: review?(bugs)
Attachment #8417946 - Flags: review?(bugs)
Flags: needinfo?(amarchesini)
(Assignee)

Comment 81

5 years ago
Flags: needinfo?(amarchesini)
(Assignee)

Comment 82

5 years ago
(Assignee)

Comment 83

5 years ago
Here the 2 patches you have to apply before pushing my 2 patches. On try: https://tbpl.mozilla.org/?tree=Try&rev=5a7447fc364a
Just a tiny bit red tryserver.
(Assignee)

Comment 86

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=8f02831c7184

This new push has a change suggested by smaug.
Comment on attachment 8417945 [details] [diff] [review]
patch 1 - WebSocketImpl

>   if (mKeepingAlive && !shouldKeepAlive) {
>     mKeepingAlive = false;
>-    static_cast<EventTarget*>(this)->Release();
>+
>+    mImpl->Release();
>+    mImpl = nullptr;
This looks wrong.
mImpl should set to null only when mImpl object is actually going away, not when
we update the mKeepingAlive flag



> void
> WebSocket::DontKeepAliveAnyMore()
> {
>   NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
>   if (mKeepingAlive) {
>     mKeepingAlive = false;
>-    static_cast<EventTarget*>(this)->Release();
>+
>+    mImpl->Release();
>+    mImpl = nullptr;
>   }
Similar here.
(Assignee)

Comment 88

5 years ago
Attachment #8421842 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 89

5 years ago
Attachment #8417946 - Attachment is obsolete: true
Attachment #8421843 - Flags: review?(bugs)
(Assignee)

Comment 90

5 years ago
Posted patch patch 1 - WebSocketImpl (obsolete) — Splinter Review
Attachment #8417945 - Attachment is obsolete: true
Attachment #8421844 - Flags: review?(bugs)

Updated

5 years ago
Attachment #8421842 - Attachment is patch: true

Comment 91

5 years ago
Comment on attachment 8421842 [details] [diff] [review]
patch 2 - WebSocketChannel::GetEffectiveURL() in IPC

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

Looks good, but one nit.  If you agree to it and fix it, feel free to mark next patch +r w/o review

::: netwerk/protocol/websocket/WebSocketChannelChild.cpp
@@ +56,5 @@
>  {
>    NS_ABORT_IF_FALSE(NS_IsMainThread(), "not main thread");
>  
>    LOG(("WebSocketChannelChild::WebSocketChannelChild() %p\n", this));
>    BaseWebSocketChannel::mEncrypted = aSecure;

So it looks like WebSocketChannelChild is currently (i.e. without your patch) not keeping mEncrypted up to date with redirects to wss://.  WebSocketChannel resets it here:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#2536

But the Child is currently only setting it in its constructor. That's a bug.  Let's also update mEncrypted in ::OnStart along with mSecure, or better yet, collapse them into one variable (I don't think it's possible for them to validly have different values).
Attachment #8421842 - Flags: review?(jduell.mcbugs) → feedback+
Comment on attachment 8421843 [details] [diff] [review]
patch 3 - WebSocket in Workers

As discussed on IRC WebSocketImpl may be deleted in non-owner-of-mParent thread, 
which causes mParent to be released in the wrong thread.
Disconnect() also must not be called in a random thread, or WebSocket::mImpl must be used in a threadsafe way. Probably the latter since Disconnect() does
some other stuff which need to be main-thread only.
Attachment #8421843 - Flags: review?(bugs) → review-
Comment on attachment 8421844 [details] [diff] [review]
patch 1 - WebSocketImpl

>+  ~WebSocketImpl()
>+  {
>+    // If we threw during Init we never called disconnect
>+    if (mDisconnected) {
Shouldn't this be !mDisconnected.


> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(WebSocket,
>                                                 DOMEventTargetHelper)
>-  tmp->Disconnect();
>-  NS_IMPL_CYCLE_COLLECTION_UNLINK(mPrincipal)
>-  NS_IMPL_CYCLE_COLLECTION_UNLINK(mURI)
>-  NS_IMPL_CYCLE_COLLECTION_UNLINK(mChannel)
>+  if (tmp->mImpl) {
>+    tmp->mImpl->Disconnect();
>+    NS_IMPL_CYCLE_COLLECTION_UNLINK(mImpl->mPrincipal)
>+    NS_IMPL_CYCLE_COLLECTION_UNLINK(mImpl->mURI)
>+    NS_IMPL_CYCLE_COLLECTION_UNLINK(mImpl->mChannel)
>+    tmp->mImpl = nullptr;
You could just assert here that mImpl is null. Disconnect() should have set it to null already.

>+WebSocketImpl::Init(JSContext* aCx,
>+                    nsIPrincipal* aPrincipal,
>+                    const nsAString& aURL,
>+                    nsTArray<nsString>& aProtocolArray)
> {
>   NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
>   MOZ_ASSERT(aPrincipal);
> 
>-  if (!PrefEnabled()) {
>-    return NS_ERROR_DOM_SECURITY_ERR;
>-  }
>+  nsRefPtr<WebSocketImpl> kungfuDeathGrip = this;
Tiny bit ugly, but I guess this is fine to ensure WebSocket::mImpl is deleted correctly if Init fails
Could you add a comment why kungfuDeathGrip is needed here.
Attachment #8421844 - Flags: review?(bugs) → review+
(Assignee)

Comment 94

5 years ago
Jduell, can you review this patch? It contains the changes you proposed plus something else, still related to WebSocketChannel. Thanks.
Attachment #8418392 - Attachment is obsolete: true
Attachment #8418393 - Attachment is obsolete: true
Attachment #8421842 - Attachment is obsolete: true
Attachment #8423815 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 95

5 years ago
Posted patch patch 1 - WebSocketImpl (obsolete) — Splinter Review
Attachment #8421844 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8423815 - Attachment description: patch - = WebSocketChannel fix → patch 0 - WebSocketChannel fix
(Assignee)

Comment 96

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=9b5654f48eee
Attachment #8421843 - Attachment is obsolete: true
Attachment #8423820 - Flags: review?(bugs)
(Assignee)

Comment 97

5 years ago
It's almost green. Fixing the remaining issues.
Comment on attachment 8423820 [details] [diff] [review]
patch 2 - WebSocket in workers

So mImpl is still accessed unsafely in several threads 
(main thread and worker thread)
Attachment #8423820 - Flags: review?(bugs) → review-
(Assignee)

Comment 99

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=db59e7b1b770
Attachment #8423820 - Attachment is obsolete: true
Attachment #8425417 - Flags: review?(bugs)
Comment on attachment 8425417 [details] [diff] [review]
patch 2 - WebSocket in workers

Please be consistent with * usage. * should go with the type, not with param/variable name.


>+  void ReferenceObject();
>+  void ReleaseObject();
These need some comments.
And should the first one be AddRefObject()


>+class DispatchEvent : public WorkerRunnable
DispatchEvent sounds very generic name, and don't we really have something like this already in workers code?




> nsresult
> WebSocketImpl::CloseConnection(uint16_t aReasonCode,
>                                const nsACString& aReasonString)
> {
>-  NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
>+  // This method can be called from the main-thread and from the worker-thread.
>+
Assert that. Perhaps MOZ_ASSERT(mWorkerPrivate || NS_IsMainThread());



>@@ -1254,34 +1844,112 @@ WebSocket::UpdateMustKeepAlive()
>       {
>         shouldKeepAlive = false;
>       }
>     }
>   }
> 
>   if (mKeepingAlive && !shouldKeepAlive) {
>     mKeepingAlive = false;
>-    mImpl->Release();
>+    mImpl->ReleaseObject();
Is this really right. We end up calling UnregisterFeature, yet WS may stay alive
(because there is a js reference to it)
and we then add a new event listener and RegisterFeature is called.
While those runnables are being processed it is possible to get stuff gced in the worker, I think


>+void
>+WebSocket::Send(JS::Handle<JSObject*> aData,
>+                ErrorResult& aRv)
Hmm, why do we need this on workers?
Some special case in bindings?



> NS_IMETHODIMP
> WebSocketImpl::GetLoadGroup(nsILoadGroup** aLoadGroup)
> {
>   *aLoadGroup = nullptr;
> 
>-  nsresult rv;
>-  nsIScriptContext* sc = mParent->GetContextForEventHandlers(&rv);
>-  nsCOMPtr<nsIDocument> doc =
>-    nsContentUtils::GetDocumentFromScriptContext(sc);
>-
>-  if (doc) {
>-    *aLoadGroup = doc->GetDocumentLoadGroup().take();
>+  if (mParent) {
>+    nsresult rv;
>+    nsIScriptContext* sc = mParent->GetContextForEventHandlers(&rv);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return rv;
>+    }
>+
>+    nsCOMPtr<nsIDocument> doc =
>+      nsContentUtils::GetDocumentFromScriptContext(sc);
>+
>+    if (doc) {
>+      *aLoadGroup = doc->GetDocumentLoadGroup().take();
>+    }
>   }
> 
>   return NS_OK;
> }
Hmm, why don't we always have a load group?
If we're about to cancel all the requests in a load group, and the ws running in a worker doesn't have one, that one
isn't cancelled? That is btw a case to test. What happens if a worker is using ws while a new page is loaded?
Or what if there is an active worker ws when stop is called?

>diff --git a/content/base/test/test_websocket.html b/content/base/test/test_websocket.html
>--- a/content/base/test/test_websocket.html
>+++ b/content/base/test/test_websocket.html
>@@ -67,17 +67,17 @@
>  * 42. non-char utf-8 sequences
>  * 43. Test setting binaryType attribute
>  * 44. Test sending/receving binary ArrayBuffer 
>  * 45. Test sending/receving binary Blob 
>  * 46. Test that we don't dispatch incoming msgs once in CLOSING state
>  * 47. Make sure onerror/onclose aren't called during close()
>  */
> 
>-var first_test = 1;
>+var first_test = 3;
er what?





>diff --git a/dom/workers/test/websocket_worker.js b/dom/workers/test/websocket_worker.js
>new file mode 100644
>--- /dev/null
>+++ b/dom/workers/test/websocket_worker.js
This has tons of same code as test_websocket.html
Could you please do hg copy?
Attachment #8425417 - Flags: review?(bugs) → review-
(Assignee)

Comment 102

5 years ago
> >+void
> >+WebSocket::Send(JS::Handle<JSObject*> aData,
> >+                ErrorResult& aRv)
> Hmm, why do we need this on workers?
> Some special case in bindings?

Because Blobs are not supported by WebIDL bindings at the moment. So we need to use a custom "Send()" method for this. Comment 63 is about this.

I'm sending a new version of this patch to try and see how it goes.
(Assignee)

Updated

5 years ago
Depends on: 1016352

Comment 103

5 years ago
Comment on attachment 8423815 [details] [diff] [review]
patch 0 - WebSocketChannel fix

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

Looks good.

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ -339,5 @@
>    // Determine if we will open connection immediately (returns true), or
>    // delay/queue the connection (returns false)
>    static void ConditionallyConnect(WebSocketChannel *ws)
>    {
> -    NS_ABORT_IF_FALSE(NS_IsMainThread(), "not main thread");

So you got rid of the assert here, yet the only function that calls this has it's own main thread assert...  Not a big deal.
Attachment #8423815 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 104

5 years ago
Attachment #8423815 - Attachment is obsolete: true
(Assignee)

Comment 105

5 years ago
Attachment #8423818 - Attachment is obsolete: true
(Assignee)

Comment 106

5 years ago
Attachment #8425417 - Attachment is obsolete: true
Attachment #8445283 - Flags: review?(bugs)
(Assignee)

Comment 107

5 years ago
> >+class DispatchEvent : public WorkerRunnable
> DispatchEvent sounds very generic name, and don't we really have something
> like this already in workers code?

I don't think we have. But in case I can move it to some better place.

> >+void
> >+WebSocket::Send(JS::Handle<JSObject*> aData,
> >+                ErrorResult& aRv)
> Hmm, why do we need this on workers?
> Some special case in bindings?

Because we don't support Blobs in WebIDL yes, so we need this custom Send() method.

> > NS_IMETHODIMP
> > WebSocketImpl::GetLoadGroup(nsILoadGroup** aLoadGroup)
> > {
> Hmm, why don't we always have a load group?

The load group is needed for get the PBrowser object in WebSocketChannelChild.
Comment on attachment 8445283 [details] [diff] [review]
patch 2 - WebSocket in workers



>   AutoJSAPI jsapi;
>-  if (NS_WARN_IF(!jsapi.InitUsingWin(GetOwner()))) {
>-    return NS_ERROR_FAILURE;
>+  Maybe<JSAutoCompartment> ac;
>+  JSContext* cx;
>+  if (NS_IsMainThread()) {
>+    nsCOMPtr<nsIGlobalObject> globalObject = do_QueryInterface(GetOwner());
>+    if (NS_WARN_IF(!globalObject)) {
>+      return NS_ERROR_FAILURE;
>+    }
>+
>+    cx = jsapi.cx();
>+    ac.construct(cx, globalObject->GetGlobalJSObject());
>+  } else {
>+    MOZ_ASSERT(impl->mWorkerPrivate);
>+    cx = impl->mWorkerPrivate->GetJSContext();
>+
>+    WorkerGlobalScope* globalScope = impl->mWorkerPrivate->GlobalScope();
>+    MOZ_ASSERT(globalScope);
>+
>+    JS::Rooted<JSObject*> jsGlobal(cx, globalScope->GetWrapper());
>+    MOZ_ASSERT(jsGlobal);
>+
>+    ac.construct(cx, jsGlobal);
>   }
So you don't Init AutoJSAPI at all?
AutoJSAPI::Init would enter to a compartment, AFAIK




>+  if (mParent) {
>+    nsIScriptContext* sc;
>+    if (!mWorkerPrivate) {
>+      nsresult rv;
>+      sc = mParent->GetContextForEventHandlers(&rv);
>+      if (NS_WARN_IF(NS_FAILED(rv))) {
>+        return rv;
>+      }
>+    } else {
>+      nsresult rv = mParent->CheckInnerWindowCorrectness();
>+      if (NS_WARN_IF(NS_FAILED(rv))) {
>+        return rv;
>+      }
I don't understand this check. 
CheckInnerWindowCorrectness() call in, possibly, worker thread?
Why would mParent (which btw should be perhaps renamed to mWebSocket or some such)
have a window as owner here?
What guarantees  WebSocketImpl::GetLoadGroup is called only on the main thread?
This certainly needs MOZ_ASSERTs




> private:
>   WebSocket(const WebSocket& x) MOZ_DELETE;   // prevent bad usage
>   WebSocket& operator=(const WebSocket& x) MOZ_DELETE;
> 
>   // Raw pointer because this WebSocketImpl is created, managed an destroyed by
>   // WebSocket.
This is from the previous patch, but s/an/and/


>+  // Note: This should not be used directly. Use GetImpl() instead.
>   WebSocketImpl* mImpl;
>+  mozilla::Mutex mMutex;
I don't understand how this setup is enough.
You just protect mImpl, but then for example
WebSocket::GetReadyState happily uses impl->mReadyState;
(via ReadState()) and I don't see what guarantees non-racy
mReadyState reading.
I believe mutex needs to be used in almost all the cases when
ws reads some state from wsimpl.


> 
>   bool mKeepingAlive;
>   bool mCheckMustKeepAlive;
> };
> 
> } //namespace dom
> } //namespace mozilla
> 









>@@ -31,24 +13,24 @@
>  * 10. client sends a message before the ws connection is established;
>  * 11. a simple hello echo;
>  * 12. client sends a message containing unpaired surrogates
>  * 13. server sends an invalid message;
>  * 14. server sends the close frame, it doesn't close the tcp connection and
>  *     it keeps sending normal ws messages;
>  * 15. server closes the tcp connection, but it doesn't send the close frame;
>  * 16. client calls close() and tries to send a message;
>- * 17. see bug 572975 - all event listeners set
>+ * 17. see bug 572975 - all event listeners set - REMOVED (requires forcegc)
>  * 18. client tries to connect to an http resource;
>  * 19. server closes the tcp connection before establishing the ws connection;
>- * 20. see bug 572975 - only on error and onclose event listeners set
>+ * 20. see bug 572975 - only on error and onclose event listeners set - REMOVED (requires forcegc)
>  * 21. see bug 572975 - same as test 17, but delete strong event listeners when
>- *     receiving the message event;
>- * 22. server takes too long to establish the ws connection;
>- * 23. should detect WebSocket on window object;
>+ *     receiving the message event - REMOVED (requires forcegc and window);
>+ * 22. server takes too long to establish the ws connection - REMOVED (requires SpecialPower object);
>+ * 23. should detect WebSocket on window object - REMOVED (requires window object);
>  * 24. server rejects sub-protocol string
>  * 25. ctor with valid empty sub-protocol array
>  * 26. ctor with invalid sub-protocol array containing 1 empty element
>  * 27. ctor with invalid sub-protocol array containing an empty element in list
>  * 28. ctor using valid 1 element sub-protocol array
>  * 29. ctor using all valid 5 element sub-protocol array
>  * 30. ctor using valid 1 element sub-protocol array with element server will
>  *     reject
>@@ -61,34 +43,49 @@
>  * 36. negative test for sending out of range close code
>  * 37. negative test for too long of a close reason
>  * 38. ensure extensions attribute is defined
>  * 39. a basic wss:// connectivity test
>  * 40. negative test for wss:// with no cert
>  * 41. HSTS
>  * 42. non-char utf-8 sequences
>  * 43. Test setting binaryType attribute
>- * 44. Test sending/receving binary ArrayBuffer 
>- * 45. Test sending/receving binary Blob 
>+ * 44. Test sending/receving binary ArrayBuffer
>+ * 45. Test sending/receving binary Blob  - REMOVED (requires SpecialPower object)
perhaps change "REMOVED" to "something like: Only in the main thread worker tests (requires SpecialPower object)"
Same thing few times.
Attachment #8445283 - Flags: review?(bugs) → review-
Doesn't XHR send readyState changes from main thread to the worker etc, so no locking is needed so much.

Comment 111

5 years ago
unhelpful
a possible workaround is to use socket.io on websocket server side and import the websocket client library using Import Scripts, for example this code works in worker script:

     importScripts('//njs.mydomain.tld/socket.io/socket.io.js');
     socket = io.connect('njs.mydomain.tld');
     socket.on('connect',function(){
          console.log("CONNECTED SUCCESSFULLY");
     });
Importing a client side library doesn't just make Web Sockets work in Web Worker threads.  Socket.io is probably falling back to XHR, which is available to workers.

Comment 113

5 years ago
Any update on this issue ?

Comment 114

5 years ago
:baku anything new on this?
or did you pause your work on it since June?

Comment 115

5 years ago
Guys, we are working on this, but Andrea and others are busy working on more important things at the moment.  Please be patient.
(Assignee)

Comment 116

5 years ago
I don't remember how the previous patch worked and it has been easier to rewrite it. I hope the review process will be easier too.
Attachment #8445283 - Attachment is obsolete: true
Attachment #8493700 - Flags: review?(bugs)
(Assignee)

Comment 118

5 years ago
a small change to make e10s happy.
Attachment #8493700 - Attachment is obsolete: true
Attachment #8493700 - Flags: review?(bugs)
Attachment #8494252 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Assignee: nobody → amarchesini
Comment on attachment 8494252 [details] [diff] [review]
patch 2 - WebSocket in workers

Would be nice to split the tests out from this patch.
> 
>+  mozilla::Mutex mMutex;
Please document for what all cases this mutex is used.

> WebSocket::WrapObject(JSContext* cx)
> {
>+  MOZ_ASSERT(mImpl);
>+
>+  if (mImpl->mWorkerPrivate) {
>+    return WebSocketBinding_workers::Wrap(cx, this);
>+  }
>+
>   return WebSocketBinding::Wrap(cx, this);
> }
Please add a comment why different binding (because of send() ?)


>+// This class is used to clear any exception.
>+class ClearException MOZ_FINAL
MOZ_STACK_CLASS ?


>+private:
>+  // Raw pointer. This worker runs synchronously.
>+  WebSocketImpl*mImpl;
missing space before mImpl;

>+  class ClearWebSocket MOZ_FINAL
MOZ_STACK_CLASS

> WebSocketImpl::Init(JSContext* aCx,
>                     nsIPrincipal* aPrincipal,
>                     const nsAString& aURL,
>-                    nsTArray<nsString>& aProtocolArray)
>+                    nsTArray<nsString>& aProtocolArray,
>+                    ErrorResult& aRv,
>+                    bool* aConnectionFailed)
> {
>-  NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
>+  AssertIsOnMainThread();
>   MOZ_ASSERT(aPrincipal);
> 
>   // We need to keep the implementation alive in case the init disconnects it
>   // because of some error.
>   nsRefPtr<WebSocketImpl> kungfuDeathGrip = this;
> 
>   mPrincipal = aPrincipal;
> 
>   // Attempt to kill "ghost" websocket: but usually too early for check to fail
>-  nsresult rv = mParent->CheckInnerWindowCorrectness();
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  aRv = mWebSocket->CheckInnerWindowCorrectness();
>+  if (NS_WARN_IF(aRv.Failed())) {
>+    return;
>+  }
> 
>   // Shut down websocket if window is frozen or destroyed (only needed for
>   // "ghost" websockets--see bug 696085)
>-  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>-  NS_ENSURE_STATE(os);
>-  rv = os->AddObserver(this, DOM_WINDOW_DESTROYED_TOPIC, true);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-  rv = os->AddObserver(this, DOM_WINDOW_FROZEN_TOPIC, true);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!mWorkerPrivate) {
>+    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+    if (NS_WARN_IF(!os)) {
>+      aRv.Throw(NS_ERROR_FAILURE);
>+      return;
>+    }
>+
>+    aRv = os->AddObserver(this, DOM_WINDOW_DESTROYED_TOPIC, true);
>+    if (NS_WARN_IF(aRv.Failed())) {
>+      return;
>+    }
>+
>+    aRv = os->AddObserver(this, DOM_WINDOW_FROZEN_TOPIC, true);
>+    if (NS_WARN_IF(aRv.Failed())) {
>+      return;
>+    }
>+  }
> 
>   unsigned lineno;
>   JS::AutoFilename file;
>   if (JS::DescribeScriptedCaller(aCx, &file, &lineno)) {
>     mScriptFile = file.get();
>     mScriptLine = lineno;
>   }
This part doesn't really make sense when we're creating WS for workers.
We should get the mScriptFile/Line on worker side and then pass to the runnable and the runnable gives them to the WSImpl.

> 
>+  nsCOMPtr<nsIURI> uri;
>+  {
>+    nsresult rv = NS_NewURI(getter_AddRefs(uri), mURI);
>+    if (NS_FAILED(rv)) {
>+      MOZ_CRASH();
>+    }
Could you add a comment here that this means either that NS_NewURI can't create an uri from
nsIURI::GetSpec or that we're OOM'ing


> WebSocket::CreateAndDispatchMessageEvent(const nsACString& aData,
>-                                         bool isBinary)
>+                                         bool aIsBinary)
> {
>   MOZ_ASSERT(mImpl);
>-  NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
>-
>-  nsresult rv = CheckInnerWindowCorrectness();
>-  if (NS_FAILED(rv))
>-    return NS_OK;
>-
>-  AutoJSAPI jsapi;
>-  if (NS_WARN_IF(!jsapi.Init(GetOwner()))) {
>-    return NS_ERROR_FAILURE;
>+  mImpl->AssertIsOnTargetThread();
>+
>+  if (NS_IsMainThread()) {
>+    AutoJSAPI jsapi;
>+    if (NS_WARN_IF(!jsapi.Init(GetOwner()))) {
>+      return NS_ERROR_FAILURE;
>+    }
>+
>+    return CreateAndDispatchMessageEvent(jsapi.cx(), aData, aIsBinary);
>   }
>-  JSContext* cx = jsapi.cx();
>+
>+  MOZ_ASSERT(mWorkerPrivate);
>+  return CreateAndDispatchMessageEvent(mWorkerPrivate->GetJSContext(), aData,
>+                                       aIsBinary);
Why you don't use AutoJSAPI on worker? I'd really prefer consistent code paths especially in case of JSAPI usage.


>     if (mImpl->mBinaryType == dom::BinaryType::Blob) {
>-      rv = nsContentUtils::CreateBlobBuffer(cx, aData, &jsData);
>-      NS_ENSURE_SUCCESS(rv, rv);
>+      nsresult rv = nsContentUtils::CreateBlobBuffer(aCx, aData, &jsData);
>+      if (NS_WARN_IF(NS_FAILED(rv))) {
>+        return rv;
>+      }
No need for this change. NS_ENSURE_SUCCESS is just fine ;)


> WebSocket::CreateAndDispatchCloseEvent(bool aWasClean,
>                                        uint16_t aCode,
>                                        const nsAString &aReason)
> {
>-  NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread");
>-
>-  nsresult rv = CheckInnerWindowCorrectness();
>-  if (NS_FAILED(rv)) {
>-    return NS_OK;
>+  MOZ_ASSERT(mImpl);
>+  mImpl->AssertIsOnTargetThread();
>+
>+  if (NS_IsMainThread()) {
>+    nsresult rv = CheckInnerWindowCorrectness();
>+    if (NS_FAILED(rv)) {
>+      return NS_OK;
>+    }
CheckInnerWindowCorrectness() is actually thread safe since mOwnerWindow should be always null in the worker threads.
Same also elsewhere.


>+{
>+    'nativeType': 'mozilla::dom::WebSocket',
>+    'headerFile': 'mozilla/dom/WebSocket.h',
>+    'workers': True,
>+}],
Please add a comment why we need this (because of the send())


Why does this look so simple :) Could still take another look and tests in a different patch, please.
Attachment #8494252 - Flags: review?(bugs) → review-
Can we at lest submit those tests to wpt?
(Assignee)

Comment 121

5 years ago
> >-  AutoJSAPI jsapi;
> >-  if (NS_WARN_IF(!jsapi.Init(GetOwner()))) {
> >-    return NS_ERROR_FAILURE;
> >+  mImpl->AssertIsOnTargetThread();
> >+
> >+  if (NS_IsMainThread()) {
> >+    AutoJSAPI jsapi;
> >+    if (NS_WARN_IF(!jsapi.Init(GetOwner()))) {
> >+      return NS_ERROR_FAILURE;
> >+    }
> >+
> >+    return CreateAndDispatchMessageEvent(jsapi.cx(), aData, aIsBinary);
> >   }
> >-  JSContext* cx = jsapi.cx();
> >+
> >+  MOZ_ASSERT(mWorkerPrivate);
> >+  return CreateAndDispatchMessageEvent(mWorkerPrivate->GetJSContext(), aData,
> >+                                       aIsBinary);
> Why you don't use AutoJSAPI on worker? I'd really prefer consistent code
> paths especially in case of JSAPI usage.

We don't need to use AutoJSAPI in workers becuase we already have the JSContext.

All the other comments are applied. New 2 patches are coming.
(Assignee)

Comment 122

5 years ago
Attachment #8494252 - Attachment is obsolete: true
Attachment #8499397 - Flags: review?(bugs)
(Assignee)

Comment 123

5 years ago
Posted patch patch 3 - tests (obsolete) — Splinter Review
Attachment #8499398 - Flags: review?(bugs)
(In reply to Andrea Marchesini (:baku) from comment #121) 
> We don't need to use AutoJSAPI in workers becuase we already have the
> JSContext.

I'd really like to hear bholley's opinion on this.
Technically we may not need it right now, but AFAIK the plan is that one must have AutoJSAPI (or some variant of it)
on stack whenever Gecko uses JSAPI.
http://mxr.mozilla.org/mozilla-central/source/dom/base/ScriptSettings.h?rev=ed7e4dde009b#166
Flags: needinfo?(bobbyholley)
Comment on attachment 8499398 [details] [diff] [review]
patch 3 - tests

>+  ws.onerror = function(e) {
>+    postMessage({type: 'status', status: false, msg: 'onrrror called!'});
rather many rrr


>@@ -31,24 +13,24 @@
>  * 10. client sends a message before the ws connection is established;
>  * 11. a simple hello echo;
>  * 12. client sends a message containing unpaired surrogates
>  * 13. server sends an invalid message;
>  * 14. server sends the close frame, it doesn't close the tcp connection and
>  *     it keeps sending normal ws messages;
>  * 15. server closes the tcp connection, but it doesn't send the close frame;
>  * 16. client calls close() and tries to send a message;
>- * 17. see bug 572975 - all event listeners set
>+ * 17. see bug 572975 - all event listeners set - Only in the main thread tests (requires forcegc)
Could you make this working by sending message to main thread, do memory-pressure, and then continue?


>- * 44. Test sending/receving binary ArrayBuffer 
>- * 45. Test sending/receving binary Blob 
>+ * 44. Test sending/receving binary ArrayBuffer
>+ * 45. Test sending/receving binary Blob  - Only in the main thread worker tests (requires SpecialPower object)
not your comment, but receiving.
Attachment #8499398 - Flags: review?(bugs) → review+
(Assignee)

Comment 126

5 years ago
This patch is going to land after DOMBlob/DOMFile ported to webidl, so we don't need a custom WebSocket::Send(). Here the same patch rebased on top of bug 1047483.
Attachment #8499397 - Attachment is obsolete: true
Attachment #8499397 - Flags: review?(bugs)
Attachment #8500973 - Flags: review?(bugs)
Comment on attachment 8500973 [details] [diff] [review]
patch 2 - WebSocket in workers

CallDispatchConnectionCloseEvents::Run has wrong assert (asserts main thread, not current thread), and since 
the tests didn't catch that, we certainly should add some test.
That might reveal some other issues, so I'd prefer to re-review after that.
Attachment #8500973 - Flags: review?(bugs)
(Assignee)

Comment 128

5 years ago
So, yeah. we don't have any test for this runnable. I'm not sure how to create such test because I have to interrupt the communication from the server in order to generate a 'onStop' calls.
I need a feedback from jduell or somebody else from necko team.
Attachment #8500973 - Attachment is obsolete: true
Attachment #8501081 - Flags: review?(bugs)
Flags: needinfo?(jduell.mcbugs)
Comment on attachment 8501081 [details] [diff] [review]
patch 2 - WebSocket in workers


> WebSocketImpl::Observe(nsISupports* aSubject,
>                        const char* aTopic,
>                        const char16_t* aData)
> {
>+  AssertIsOnTargetThread();
Shouldn't this check MainThread, not TargetThread?

>+
>   if ((mReadyState == WebSocket::CLOSING) ||
>       (mReadyState == WebSocket::CLOSED)) {
>     return NS_OK;
>   }
> 
>   nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aSubject);
>-  if (!mParent->GetOwner() || window != mParent->GetOwner()) {
>+  if (!mWebSocket->GetOwner() || window != mWebSocket->GetOwner()) {
>     return NS_OK;
>   }
> 
>   if ((strcmp(aTopic, DOM_WINDOW_FROZEN_TOPIC) == 0) ||
>       (strcmp(aTopic, DOM_WINDOW_DESTROYED_TOPIC) == 0))
>   {
>     CloseConnection(nsIWebSocketChannel::CLOSE_GOING_AWAY);
>   }
So what happens to Websocket connections in dedicated workers when the page goes to bfcache?


> WebSocketImpl::GetLoadGroup(nsILoadGroup** aLoadGroup)
> {
>+  AssertIsOnMainThread();
>+
>   *aLoadGroup = nullptr;
> 
>   nsresult rv;
>-  nsIScriptContext* sc = mParent->GetContextForEventHandlers(&rv);
>+  nsIScriptContext* sc = mWebSocket->GetContextForEventHandlers(&rv);
>   nsCOMPtr<nsIDocument> doc =
>     nsContentUtils::GetDocumentFromScriptContext(sc);
> 
>   if (doc) {
>     *aLoadGroup = doc->GetDocumentLoadGroup().take();
>   }
> 
>   return NS_OK;
> }
So in workers we have always null loadgroup?
At least in dedicated worker we should get load group from the parent window I think.






+
>+class DispatchEvent MOZ_FINAL : public WorkerRunnable
DispatchEvent is a bit too commonly use name, especially because EventTarget DOM interface has dispatchEvent method.
Rename to something else.... maybe WorkerRunnableDispatcher
Attachment #8501081 - Flags: review?(bugs) → review-
(Assignee)

Comment 130

5 years ago
The bfcache in workers and the loadGroup in workers are not tested.
We have to find a way to write a mochitest for these 2 issues if we care.
Attachment #8501081 - Attachment is obsolete: true
Attachment #8501243 - Flags: review?(bugs)
(Assignee)

Comment 131

5 years ago
Attachment #8501243 - Attachment is obsolete: true
Attachment #8501243 - Flags: review?(bugs)
Attachment #8501870 - Flags: review?(bugs)
(Assignee)

Comment 132

5 years ago
I added a test for the loadGroup issue.
Attachment #8499398 - Attachment is obsolete: true
Attachment #8501871 - Flags: review?(bugs)
Comment on attachment 8501871 [details] [diff] [review]
patch 3 - tests

>+  worker.onmessage = function(e) {
>+    if (e.data == 'opened') {
>+      stopped = true;
>+      window.stop();
>+    } else if (e.data == 'closed') {
>+      ok(stopped, "Good!");
>+      runTest();
Shouldn't you set stopped back to false before calling runTests();
Attachment #8501871 - Flags: review?(bugs) → review+
Comment on attachment 8501870 [details] [diff] [review]
patch 2 - WebSocket in workers

> WebSocketChannel::BeginOpen()
> {
>+  if (!NS_IsMainThread()) {
>+    NS_DispatchToMainThread(
>+      NS_NewRunnableMethod(this, &WebSocketChannel::BeginOpen),
>+      NS_DISPATCH_NORMAL);
Odd indentation for NS_DISPATCH_NORMAL. Could be under 'this'


r+ if AutoJSAPI is used also in workers, or bholley says one shouldn't use it in workers.

(AutoJSAPI changes could be done on top of this patch to ease reviewing.)
Attachment #8501870 - Flags: review?(bugs) → review+
(Assignee)

Comment 136

5 years ago
Attachment #8503135 - Flags: review?(bugs)
Attachment #8503135 - Flags: review?(bugs) → review+
(Assignee)

Comment 138

5 years ago
Release Note Request (optional, but appreciated)
[Why is this notable]: WebSocket in workers is a nice feature that is extremely useful for games and complex webapps.
relnote-firefox: --- → ?
Awesome! Thanks Andrea! This is quite exciting to have!

Comment 141

5 years ago
Finally! Thank you so much for implementing this Andrea and thank you Olli for reviewing it.

I'm so happy: finally games like Runescape, that need websockets in webworkers, can run in the browser natively without Java.

Gonna test the nightly now :)
(In reply to Olli Pettay [:smaug] from comment #124)
> I'd really like to hear bholley's opinion on this.
> Technically we may not need it right now, but AFAIK the plan is that one
> must have AutoJSAPI (or some variant of it)

Eventually AutoJSAPI will be the only way to get a JSContext, so a pre-existing JSContext will mean we already have one. But workers haven't been fully fixed yet - see bug 1072144.
Flags: needinfo?(bobbyholley)
Don't quite understand that comment.
Of course there is the pre-existing JSContext in workers, that is what WorkerPrivate::GetJSContext()
gives you. But should we not use that, but just AutoJSAPI + WorkerPrivate::GlobalScope() ?
Flags: needinfo?(bobbyholley)
(In reply to Olli Pettay [:smaug] from comment #144)
> Don't quite understand that comment.
> Of course there is the pre-existing JSContext in workers, that is what
> WorkerPrivate::GetJSContext()
> gives you. But should we not use that, but just AutoJSAPI +
> WorkerPrivate::GlobalScope() ?

If the caller is passing a JSContext, we'll get an AutoJSAPI there when do we bug 1072144. But if this is shared code and the mainthread codepath is using AutoJSAPI, I agree that we should be consistent for both.
Flags: needinfo?(bobbyholley)
Depends on: 1082845
Added to the release notes with the wording "WebSocket available in Workers"
I just tried web socket in a shared web worker in the latest Firefox Nightly(v36), and I got a `NS_ERROR_FAILURE` error. Please note that the name resolves perfectly fine everywhere else, even within Firefox.
The line it threw the error had this:
instance = new WebSocket(uri);
where value of uri was "wss://mydomain.tld/live/?EIO=3&transport=websocket&sid=F0uG77boysE-pvCnAAAc"
(In reply to aneesiqbalbhatti from comment #147)
> I just tried web socket in a shared web worker in the latest Firefox
> Nightly(v36), and I got a `NS_ERROR_FAILURE` error.
Could you report a new bug? Thanks
(Assignee)

Comment 149

5 years ago
And assign that bug to me. Thanks.
Depends on: 1090183
Depends on: 1090198
Depends on: 1089328
No longer depends on: 1090198
Depends on: 1090142
Depends on: 1090973
Depends on: 1091962
Depends on: 1105194

Updated

4 years ago
Duplicate of this bug: 1106709
Depends on: 1111971
(Assignee)

Comment 152

4 years ago
We decided to disable this feature by pref for ff35.
The docs team will proceed under the assumption, for now, that this will ship preffed on in Firefox 36.

I've already added mention of this feature to the main WebSocket API page's compatibility table: https://developer.mozilla.org/en-US/docs/WebSockets#Browser_compatibility

The documentation changes that remain to be made are:

* Update Firefox X for developers and Firefox 36 for developers if this slides out further than Firefox 36.

* There appear to be no API syntax changes: no new or changed interfaces, methods, or properties.

* Reference pages need to be marked that the API can be used in workers.

* Guides and tutorial documentation need to be updated to include examples of and discussion of using WebSocket in a worker.

I'm not including URLs here because this documentation work will be handled as part of the overall WebSocket doc revamp, which includes moving the content we already have to a new location. See bug 1115091 for that work.
(In reply to Eric Shepherd [:sheppy] from comment #153)
> The docs team will proceed under the assumption, for now, that this will
> ship preffed on in Firefox 36.

That's the plan :)
relnoted as "WebSocket available in Workers"
Why did we put this for release notes for 37 rather than 36?
Because this certainly won't be in 36.
Thanks for the clarification.
Target Milestone: mozilla35 → mozilla37
People should really set the "disabled" state for branches when we disable there...
Target Milestone: mozilla37 → mozilla35

Updated

4 years ago
Flags: needinfo?(jduell.mcbugs)
Olli pointed out to me that this feature has been disabled in 37. (bug 1121406#c8) I have updated the relnote to 38.

Jean-Yves - We should push this feature to the 38 dev notes.
Flags: needinfo?(jypenator)
Depends on: 1150805
Clearing ni/. The Fx 38/37 for devs page have been updated long ago.
Flags: needinfo?(jypenator)
You need to log in before you can comment on or make changes to this bug.