Note: There are a few cases of duplicates in user autocompletion which are being worked on.

e10s: Remote WebSockets

VERIFIED FIXED in Firefox 7

Status

()

Core
Networking: WebSockets
VERIFIED FIXED
8 years ago
6 years ago

People

(Reporter: jduell, Assigned: jdm)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Other Branch
mozilla7
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus +

Firefox Tracking Flags

(firefox7 fixed, fennec7+)

Details

Attachments

(3 attachments, 17 obsolete attachments)

3.02 KB, patch
cmtalbert
: review+
Details | Diff | Splinter Review
23.15 KB, patch
Details | Diff | Splinter Review
70.18 KB, patch
jduell
: review+
Details | Diff | Splinter Review
This is not a must-have (or even a strong want-have) for the next fennec release, but at some point we'll want to make web sockets work under e10s.  We'll want a separate IPDL protocol (PWebSocket) for it.

Hopefully it won't be much work, as the trickier stuff (like authentication) should be similar enough to whatever we cook up for HTTP, and web socket messages should map pretty straightforwardly onto IPDL msgs or shmem.
(Assignee)

Updated

7 years ago
Duplicate of this bug: 581033

Updated

7 years ago
tracking-fennec: --- → 2.0b2+

Updated

7 years ago
Assignee: nobody → jduell.mcbugs
tracking-fennec: 2.0b2+ → 2.0-
Duplicate of this bug: 601582

Comment 3

7 years ago
I think it's a must-have, at least for parity with Fx4 Desktop.

Comment 4

7 years ago
I'm confused. I saw Paul demo using Web Sockets to control a presentation in another computer just a couple of weeks ago. Did e10s disable that feature?

Comment 5

7 years ago
@Lars: Yes:

Error: docShell: Not Remotable
Source File: chrome://global/content/bindings/browser.xml
Firefox can't establish a connection to the server at ws://.../
(Assignee)

Comment 6

7 years ago
I'll see what I can do with this.
Assignee: jduell.mcbugs → josh

Updated

7 years ago
Summary: e10s: Web Sockets → e10s: Remote WebSockets

Comment 7

7 years ago
Sorry for this naive comment. I don't really understand all the details of this
discussion (for instance, I'm not really sure of what e10s is) so if my remark is
stupid, please, just ignore it.

If there is no plan to integrate correctly the websockets for the next stable
Fennec version, could it be possible to disable them by default (network.websocket.enabled set to false)? This will give a change to applications that need server pushes to switch smoothly to another implementation method such multipart XHRs.

Comment 8

7 years ago
Just another comment that might interest you. If you are looking for incentive
for adding websockets to Fennec you might find interesting watching this video.
Go to the time position 7:03 and you will see the demo. Of course, everything is implemented with Firefox and Fennec (this is acknowledged at the end of the talk).
Thanks to all of you.

http://www.college-de-france.fr/default/EN/all/cha_inf2009/Cours_et_seminaire_du_20_janvi.jsp
OS: Linux → All
Hardware: x86 → All
(Reporter)

Comment 9

7 years ago
> If there is no plan to integrate correctly the websockets for the next stable
Fennec version, could it be possible to disable them by default
(network.websocket.enabled set to false)?

Yes--we should check this in now, in case there's any fennec release before we ship e10 websockets.

Updated

7 years ago
tracking-fennec: 2.0- → 2.0+
(In reply to comment #7)
> If there is no plan to integrate correctly the websockets for the next stable
> Fennec version, could it be possible to disable them by default
> (network.websocket.enabled set to false)?

Even with this pref change, sites still seem to think that Fennec supports web sockets.  The window.WebSocket property still exists.  Is this a bug?
(Assignee)

Comment 11

7 years ago
I'm inclined to think that window.WebSocket existing is not a bug.  For one thing, that's controlled by the DOMClassInfo stuff as far as I can tell, and I don't believe that's easy to hide based on a pref.  For another, we check the pref during websocket initialization and throw a NS_ERROR_DOM_SECURITY_ERROR - I have no idea if this is to spec and people should be catching this and simply aren't.

Comment 12

7 years ago
Just for clarity, this is a blocker for release.

Comment 13

7 years ago
> This is not a must-have (or even a strong want-have)

At the moment there is no websockets + canvas solution on Android.  When there is one, it'll extend realtime network-aware UIs that would previously have needed a native applet / app to Android.  There are people who "strongly want to have" it ;-) and I hope it'll be possible to ship with it.
(Assignee)

Comment 14

7 years ago
I'm working on the implementation right now, and the bug is marked as blocking the final release of mobile Firefox.  There is no need for advocacy.
(Assignee)

Comment 15

7 years ago
Created attachment 491957 [details] [diff] [review]
WIP

This patch allows sites like http://websockets.org/echo.html and http://mrdoob.com/projects/multiuserpad to work seemingly correctly.  There are still problems with the implementation (for example, cookies don't work yet), but I think it's the right architecture.
(Assignee)

Updated

7 years ago
Attachment #491957 - Attachment is obsolete: true
(Assignee)

Comment 16

7 years ago
Created attachment 492835 [details] [diff] [review]
WIP v2

Rebased on top of bug 574897. Cookies should now work, and theoretically GetInterface for an auth prompt on the channel should also work.  I'm stumped about what to do with the nsILoadContext block that bug 574897 added in GetInterface, however.
(Assignee)

Updated

7 years ago
Depends on: 574897
(Reporter)

Updated

7 years ago
Assignee: josh → nobody
Component: Networking → Networking: WebSockets
QA Contact: networking → networking.websockets
(Reporter)

Updated

7 years ago
Assignee: nobody → josh
jdm, should someone be looking at this wip patch?
(Assignee)

Comment 18

7 years ago
Comment on attachment 492835 [details] [diff] [review]
WIP v2

I suppose it wouldn't hurt if jduell or anybody else knowledgeable wanted to look over it.
Attachment #492835 - Flags: feedback?(jduell.mcbugs)
I could take a look, if anyone is going to do this sooner, please drop a note.
(Assignee)

Updated

7 years ago
Depends on: 615745
(Assignee)

Updated

7 years ago
Attachment #492835 - Attachment is obsolete: true
Attachment #492835 - Flags: feedback?(jduell.mcbugs)
(Assignee)

Comment 20

7 years ago
Created attachment 494244 [details] [diff] [review]
Add cross-process websocket support.
(Assignee)

Comment 21

7 years ago
Now passes all cookie tests in bug 574897 with the help of SpecialPowers.
(Assignee)

Updated

7 years ago
Attachment #494244 - Flags: feedback?(honzab.moz)
Blocks: 616348
(Assignee)

Updated

7 years ago
Attachment #494244 - Attachment is obsolete: true
Attachment #494244 - Flags: feedback?(honzab.moz)
(Assignee)

Comment 22

7 years ago
Created attachment 495639 [details] [diff] [review]
Add cross-process websocket support.
(Assignee)

Comment 23

7 years ago
Comment on attachment 495639 [details] [diff] [review]
Add cross-process websocket support.

Alright, I think this is good to go.  I've verified that no objects leak, random browsing of websockets-enabled websites works flawlessly, all websockets tests in the tree pass, and I've got a comfortable mental model of how the various proxy and concrete objects on the parent and child interact.  Honza, if you're not the right person for this then please reassign.
Attachment #495639 - Flags: review?(honzab.moz)
(Assignee)

Comment 24

7 years ago
Honza, looks like this can wait until bug 616733 is resolved one way or the other.
Depends on: 616733
As I understand it, firefox is pref'ing this off for 4.0, so this shouldn't block the release anymore. If my understanding is incorrect, please leave a comment. 

Also, to be clear, I don't see any reason for this not to land if the patches are ready.
tracking-fennec: 2.0+ → 2.0-
tracking-fennec: 2.0- → 2.0next+
honza any update on this review?
(Assignee)

Comment 27

6 years ago
Comment on attachment 495639 [details] [diff] [review]
Add cross-process websocket support.

This review isn't worth Honza's time. The websocket design has changed significantly in the new implementation, so I (or someone else) needs to sit down an reevaluate how best to remote them. In fact, I think I'll try to do that during the train rides I'll be taking in the next few days.
Attachment #495639 - Flags: review?(honzab.moz)
(Assignee)

Updated

6 years ago
Depends on: 640003
(Assignee)

Comment 28

6 years ago
Hooray, it looks like remoting the new websockets patch should be significantly easier.  We'll want a proxy listener object in the parent process that relays to the child, and some kind of proxy protocol handler in the child that relays the initial AsyncOpen to the parent, I think. I haven't sorted out all the details, but I'm hopeful. I'd be happy to put together a patch when I start full time at the end of May.
(Reporter)

Comment 29

6 years ago
I think we're going to want to move on this sooner than the end of May, assuming that biesi will get a chance to review the main websockets patches before then, and that we want to land e10s support concurrently (I don't care personally about landing them at the same time, but I seem to remember other people feeling it was a requirement).

jdm:  please unassign yourself if you don't think you can get to this in a few weeks or less, and we'll find someone to do it.
(Assignee)

Comment 30

6 years ago
Turns out that I've spent the majority of the past couple days on trains, so I'll be able to put up a proof-of-concept semi-patch in the near future. We can see where to go from there.
(Assignee)

Comment 31

6 years ago
Created attachment 529807 [details] [diff] [review]
Make websockets work across processes.

This patch is conceptually complete. I've manually put together a diff based off of the most recent proposed websockets patch, so it may or may not apply to any given tree. It also conceptually builds, as I've gone to fairly painstaking trouble of reading over all my code changes. If someone wants to take it any run with it from here, that'd be swell.
Attachment #495639 - Attachment is obsolete: true
tracking-fennec: 2.0next+ → 6+
(Assignee)

Comment 32

6 years ago
Created attachment 529865 [details] [diff] [review]
Remote websockets.

I got access to a real tree, so this is actually a patch that can be applied. I also moved the IPC classes into new files, because the originals were long and complicated enough already.
Attachment #529807 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
No longer depends on: 574897
(Assignee)

Comment 33

6 years ago
Created attachment 529987 [details] [diff] [review]
Remote websockets.

Now buildable, thanks to a few hours spent in the Paris office.
Attachment #529865 - Attachment is obsolete: true
(Assignee)

Comment 34

6 years ago
Created attachment 530102 [details] [diff] [review]
Pqtch
Attachment #529987 - Attachment is obsolete: true
(Assignee)

Comment 35

6 years ago
The attached patch represents the sum of my work. I'm not sure how much else I'll be able to do before I return on the 29th, so I'm unassigning myself. My work was at the point where I was attempting to test fennec against updated websocket tests to see what was still broken, last I recall.
Assignee: josh → nobody
tracking-fennec: 6+ → 7+
(Assignee)

Comment 36

6 years ago
Created attachment 534623 [details] [diff] [review]
Remote websockets.

Psyke! Here's a patch that passes all the websocket tests in the tree. The only remaining issues are that I haven't added any new logging, and the mochitest currently leaks like a bucket with no bottom.
Attachment #530102 - Attachment is obsolete: true
(Assignee)

Comment 37

6 years ago
Created attachment 534625 [details] [diff] [review]
Use SpecialPowers in tests

You can never have too many GC methods in SpecialPowers.
Attachment #534625 - Flags: review?(ctalbert)
(Assignee)

Comment 38

6 years ago
Created attachment 535997 [details] [diff] [review]
Remote websockets.
(Assignee)

Comment 39

6 years ago
Created attachment 535998 [details] [diff] [review]
Update websocket test to use SpecialPowers.
(Assignee)

Updated

6 years ago
Attachment #534623 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #534625 - Attachment is obsolete: true
Attachment #534625 - Flags: review?(ctalbert)
(Assignee)

Updated

6 years ago
Attachment #535998 - Flags: review?(ctalbert)
(Assignee)

Comment 40

6 years ago
Comment on attachment 535997 [details] [diff] [review]
Remote websockets.

This patch is the best one to date! Now with fewer leaks, fewer crashes and more logging. All mochitests also pass with flying colours. Jason, do you feel like taking this?
Attachment #535997 - Flags: review?(jduell.mcbugs)

Updated

6 years ago
Attachment #535998 - Flags: review?(ctalbert) → review+

Updated

6 years ago
Assignee: nobody → josh
(Reporter)

Comment 41

6 years ago
Comment on attachment 535997 [details] [diff] [review]
Remote websockets.

jdm already discussed much of this on IRC, but I've found some new things.   I'll want to have a look at the next patch too.

Test cases: I haven't looked at the mochitest(s), but let's make sure we cover
- bogus URI in websocket constructor
- valid but refused/unavailable URI
- sync XHR in onmessage() (have server close WS: make sure readyState doesn't change to CLOSED during onmessage()).
- test that involves an HTTP redirect (both failed and succeeded redirects would be good)

>diff --git a/netwerk/build/nsNetModule.cpp b/netwerk/build/nsNetModule.cpp
>
>+static BaseWebSocketHandler*
>+WebSocketHandlerConstructor(bool aSecure)
>+{
>+  if (XRE_GetProcessType() != GeckoProcessType_Default) {
>+      return new nsWebSocketHandlerChild(aSecure);
>+  }

Use IsNeckoChild()?  Are you sure you want the check to be !GeckoProcessType_Default, or is ==GeckoProcessType_Content good enough?  We're only supporting tab processes, right?  In which case I'd think IsNeckoChild() should work fine.  

>diff --git a/netwerk/protocol/websocket/PWebSocket.ipdl b/netwerk/protocol/websocket/PWebSocket.ipdl
>new file mode 100644

Your new files seem to be using DOS line-endings (I see lots of ^M in vim).  Convert to unix (dos2unix is one util for doing that).

Also, not all of your new files have emacs/vim modelines.

>+  // Forwarded methods corresponding to methods on nsIWebSocketProtocolHandler

s/nsIWebSocketProtocolHandler/nsIWebSocketListener/

>diff --git a/netwerk/protocol/websocket/nsWebSocketHandler.cpp b/netwerk/protocol/websocket/nsWebSocketHandler.cpp

>+#include "mozilla/net/NeckoChild.h"

See comments on logging below.  Also may need to remove #include of prlog.h below this.

> #if defined(PR_LOGGING)
>-static PRLogModuleInfo *webSocketLog = nsnull;
>+PRLogModuleInfo *webSocketLog = nsnull;
> #endif
> #define LOG(args) PR_LOG(webSocketLog, PR_LOG_DEBUG, args)

Hmm, we're going to want logging on when !DEBUG, which means we need to have FORCE_PR_LOG on AFICT.  Look at how nsHttp.h does it in lines 43-61.  I'm surprised we don't run into an error re-#defining LOG here--it's probably a warning?  Anyway, I think you'll want to centralize this in BaseWebChannel.h.  Make sure to compile an opt build and make sure that logging works in non-debug mode. 

Our logging is a real mess.

>diff --git a/netwerk/protocol/websocket/nsWebSocketHandler.h b/netwerk/protocol/websocket/nsWebSocketHandler.h
> 
>+class BaseWebSocketHandler : public nsIWebSocketProtocol,

As mentioned on IRC, rename "BaseWebSocketChannel" (per bug 664860) and move into its own .h/.cpp files.

>+public:
>+    BaseWebSocketHandler();
>+
>+    NS_DECL_NSIPROTOCOLHANDLER
>+
>+    NS_SCRIPTABLE NS_IMETHOD QueryInterface(const nsIID & uuid, void **result NS_OUTPARAM) = 0;
>+    NS_IMETHOD_(nsrefcnt ) AddRef(void) = 0;
>+    NS_IMETHOD_(nsrefcnt ) Release(void) = 0;

As mentioned on IRC, I don't think you need to declare methods you're inheriting from interfaces unless you're actually defining them, so these can be eliminated.

>+
>+    NS_SCRIPTABLE NS_IMETHOD GetOriginalURI(nsIURI **aOriginalURI);
>+    NS_SCRIPTABLE NS_IMETHOD GetURI(nsIURI **aURI);
>+    NS_SCRIPTABLE NS_IMETHOD GetNotificationCallbacks(nsIInterfaceRequestor **aNotificationCallbacks);
>+    NS_SCRIPTABLE NS_IMETHOD SetNotificationCallbacks(nsIInterfaceRequestor *aNotificationCallbacks);

80 columns max, please.  Maybe break after NS_IMETHOD?  I'm not used to having NS_SCRIPTABLE in the decls, which makes lines (and even the column of the 1st param) very long.  We can probably scrap the NS_SCRIPTABLE decls--we don't use them in necko except a few scattered places, and they're just for dehydra analysis.  

>+    NS_SCRIPTABLE NS_IMETHOD GetSecurityInfo(nsISupports **aSecurityInfo) = 0;
>+
>+    NS_SCRIPTABLE NS_IMETHOD AsyncOpen(nsIURI *aURI,
>+                         const nsACString &aOrigin,
>+                         nsIWebSocketListener *aListener,
>+                         nsISupports *aContext) = 0;
>+    NS_SCRIPTABLE NS_IMETHOD Close() = 0;
>+    NS_SCRIPTABLE NS_IMETHOD SendMsg(const nsACString &aMsg) = 0;
>+    NS_SCRIPTABLE NS_IMETHOD SendBinaryMsg(const nsACString &aMsg) = 0;

I think these pure virtual decls can also be eliminated.

>+protected:
>+    nsCOMPtr<nsIURI>                mOriginalURI;
>+    nsCOMPtr<nsIURI>                mURI;
>+    nsCOMPtr<nsIWebSocketListener>  mListener;
>+    nsCOMPtr<nsISupports>           mContext;
>+    nsCOMPtr<nsIInterfaceRequestor> mCallbacks;
>+    nsCOMPtr<nsILoadGroup>          mLoadGroup;
>+
>+    nsCString                       mProtocol;
>+    nsCString                       mOrigin;
>+
>+    PRBool  mEncrypted;

indent mEncrypted to same level as other data members.

> public:
>-    nsWebSocketSSLHandler() {nsWebSocketHandler::mEncrypted = PR_TRUE;}
>+    nsWebSocketSSLHandler() {BaseWebSocketHandler::mEncrypted = PR_TRUE;}

Put space between { and BaseWebSocketHandler, and btw ; and }

>diff --git a/netwerk/protocol/websocket/nsWebSocketHandlerIPC.cpp b/netwerk/protocol/websocket/nsWebSocketHandlerIPC.cpp

per IRC, split each class into its own file, and name WebSocketChannelChild|Parent.  And we need ChannelEventQueue for all incoming IPDL traffic to child.

>+bool
>+nsWebSocketHandlerParent::RecvDeleteSelf()
>+{
>+  LOG(("nsWebSocketHandlerParent::RecvDeleteSelf() %p\n", this));
>+  mProtocol = nsnull;
>+  mAuthProvider = nsnull;
>+  return Send__delete__(this);
>+}

Send__delete__ should be guarded with mIPCOpen.  The child process could conceivably drop dead at any point.

>+
>+bool
>+nsWebSocketHandlerParent::RecvAsyncOpen(const IPC::URI& aURI,
>+                                        const nsCString& aOrigin,
>+                                        const nsCString& aProtocol,
>+                                        const bool& aSecure)
>+{
>+  LOG(("nsWebSocketHandlerParent::RecvAsyncOpen() %p\n", this));
>+  nsresult rv;
>+  if (aSecure) {
>+    mProtocol =
>+        do_CreateInstance("@mozilla.org/network/protocol;1?name=wss", &rv);
>+  } else {
>+    mProtocol =
>+        do_CreateInstance("@mozilla.org/network/protocol;1?name=ws", &rv);

indent do_CreateInstance by 2, not 4

>+bool
>+nsWebSocketHandlerParent::CancelEarly()
>+{
>+  LOG(("nsWebSocketHandlerParent::CancelEarly() %p\n", this));
>+  return SendAsyncOpenFailed();
>+}

Also guard this with mIPCOpen.

>diff --git a/netwerk/protocol/websocket/nsWebSocketHandlerIPC.h b/netwerk/protocol/websocket/nsWebSocketHandlerIPC.h
>+
>+class nsWebSocketHandlerParent : public PWebSocketParent,
>+                                 public nsIWebSocketListener,
>+                                 public nsIInterfaceRequestor
>+{
>+private:
>+  bool RecvAsyncOpen(const IPC::URI& aURI,
>+                     const nsCString& aOrigin,
>+                     const nsCString& aProtocol,
>+                     const bool& aSecure);
>+  bool RecvClose();
>+  bool RecvSendMsg(const nsCString& aMsg);
>+  bool RecvSendBinaryMsg(const nsCString& aMsg);
>+  bool RecvDeleteSelf();
>+  bool CancelEarly();
>+
>+  void ActorDestroy(ActorDestroyReason why);
>+
>+  nsCOMPtr<nsIAuthPromptProvider> mAuthProvider;
>+  nsCOMPtr<nsIWebSocketProtocol> mProtocol;

In anticipation of bug 664860 (and to avoid confusion with the Base class' String mProtocol field), let's rename mProtocol -> mChannel.
Attachment #535997 - Flags: review?(jduell.mcbugs) → review-
(Assignee)

Comment 42

6 years ago
(In reply to comment #41)

>>+  // Forwarded methods corresponding to methods on nsIWebSocketProtocolHandler
>
>s/nsIWebSocketProtocolHandler/nsIWebSocketListener/

Nope, these are from the ProtocolHandler (which is actually the channel).

> >diff --git a/netwerk/protocol/websocket/nsWebSocketHandler.cpp b/netwerk/protocol/websocket/nsWebSocketHandler.cpp
> 
> >+#include "mozilla/net/NeckoChild.h"
> 
> See comments on logging below.  Also may need to remove #include of prlog.h
> below this.
> 
> > #if defined(PR_LOGGING)
> >-static PRLogModuleInfo *webSocketLog = nsnull;
> >+PRLogModuleInfo *webSocketLog = nsnull;
> > #endif
> > #define LOG(args) PR_LOG(webSocketLog, PR_LOG_DEBUG, args)
> 
> Hmm, we're going to want logging on when !DEBUG, which means we need to have
> FORCE_PR_LOG on AFICT.  Look at how nsHttp.h does it in lines 43-61.  I'm
> surprised we don't run into an error re-#defining LOG here--it's probably a
> warning?  Anyway, I think you'll want to centralize this in
> BaseWebChannel.h.  Make sure to compile an opt build and make sure that
> logging works in non-debug mode. 
> 
> Our logging is a real mess.

I really want to keep these sorts of things out of header files, because it just perpetuates the mess. What I've done in my updated patch is add LOG defines per file as needed, store webSocketLog in BaseWebSocketChannel.cpp, and add extern definitions to the other consumers, which is an acceptable duplication/cleanliness tradeoff in my opinion.
 
> >diff --git a/netwerk/protocol/websocket/nsWebSocketHandler.h b/netwerk/protocol/websocket/nsWebSocketHandler.h
> >+public:
> >+    BaseWebSocketHandler();
> >+
> >+    NS_DECL_NSIPROTOCOLHANDLER
> >+
> >+    NS_SCRIPTABLE NS_IMETHOD QueryInterface(const nsIID & uuid, void **result NS_OUTPARAM) = 0;
> >+    NS_IMETHOD_(nsrefcnt ) AddRef(void) = 0;
> >+    NS_IMETHOD_(nsrefcnt ) Release(void) = 0;
> 
> As mentioned on IRC, I don't think you need to declare methods you're
> inheriting from interfaces unless you're actually defining them, so these
> can be eliminated.

Turns out we refuse to link without these here. It might be an ambiguity thing since it's inheriting from multiple nsISupports bases?
 
> >+bool
> >+nsWebSocketHandlerParent::RecvDeleteSelf()
> >+{
> >+  LOG(("nsWebSocketHandlerParent::RecvDeleteSelf() %p\n", this));
> >+  mProtocol = nsnull;
> >+  mAuthProvider = nsnull;
> >+  return Send__delete__(this);
> >+}
> 
> Send__delete__ should be guarded with mIPCOpen.  The child process could
> conceivably drop dead at any point.

Well, any point is a bit much. I mean, I guess if nulling out a member triggered a destructor which spun the event loop... (ok, I'll fix it.)
(Assignee)

Comment 43

6 years ago
Created attachment 540111 [details] [diff] [review]
Remote websockets.

Haven't done anything further with tests, but this makes all the other requested changes.
(Assignee)

Comment 44

6 years ago
From test_websocket.html comments:

>Test cases: I haven't looked at the mochitest(s), but let's make sure we cover
>- bogus URI in websocket constructor

 *  1. client tries to connect to a http scheme location;
 *  4. client tries to connect using a relative url;

>- valid but refused/unavailable URI

 *  3. client tries to connect to an non-existent ws server;
 *  5. client uses an invalid protocol value;

>- sync XHR in onmessage() (have server close WS: make sure readyState doesn't change to CLOSED during onmessage()).
>- test that involves an HTTP redirect (both failed and succeeded redirects would be good)

I'll see what I can do about these last two.
(Reporter)

Updated

6 years ago
Attachment #540111 - Flags: review?(jduell.mcbugs)
(Reporter)

Updated

6 years ago
Attachment #535997 - Attachment is obsolete: true
(Assignee)

Comment 45

6 years ago
>Hmm, we're going to want logging on when !DEBUG, which means we need to have >FORCE_PR_LOG on AFICT.  Look at how nsHttp.h does it in lines 43-61.  I'm >surprised we don't run into an error re-#defining LOG here--it's probably a >warning?  Anyway, I think you'll want to centralize this in BaseWebChannel.h.  >Make sure to compile an opt build and make sure that logging works in non-debug >mode. 

NSPR logging is not part of opt builds.
(Assignee)

Comment 46

6 years ago
Created attachment 540579 [details] [diff] [review]
Test to make sure websocket callbacks don't reenter.

Reentrancy test.
Attachment #540579 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

6 years ago
Attachment #535998 - Attachment is obsolete: true
(Assignee)

Comment 47

6 years ago
Created attachment 540580 [details] [diff] [review]
Test to make sure websocket callbacks don't reenter.
Attachment #540580 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

6 years ago
Attachment #540579 - Attachment is obsolete: true
Attachment #540579 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 48

6 years ago
Created attachment 540609 [details] [diff] [review]
Remote websockets.

Now with working nspr logging.
Attachment #540609 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

6 years ago
Attachment #540111 - Attachment is obsolete: true
Attachment #540111 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 49

6 years ago
Created attachment 542313 [details] [diff] [review]
Test to make sure websocket callbacks don't reenter.

Test updated to pass with new protocol restrictions. I also confirmed that it does in fact fail without the IPDL buffering present.
(Assignee)

Updated

6 years ago
Attachment #540580 - Attachment is obsolete: true
Attachment #540580 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

6 years ago
Attachment #542313 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

6 years ago
Attachment #535998 - Attachment is obsolete: false
(Reporter)

Updated

6 years ago
Blocks: 668126
(Reporter)

Comment 50

6 years ago
Created attachment 543566 [details] [diff] [review]
DON'T LAND: Re-entrancy test, with change of wording

Nice test.  So good, in fact, that it breaks non-e10s WS.  (did you check?  It breaks for me, at least, with 

  TEST-UNEXPECTED-FAIL |  | onclose called before onmessage!  (I changed wording--yours was wrong?)

  TEST-UNEXPECTED-FAIL |  | readystate should not be CLOSED - didn't expect 3, but got it
TEST-UNEXPECTED-FAIL |  | got a close callback before finishing onmessage

Not checking in--I'll move to a new bug.
Attachment #542313 - Attachment is obsolete: true
Attachment #543566 - Flags: review+
Attachment #542313 - Flags: review?(jduell.mcbugs)
(Reporter)

Updated

6 years ago
Blocks: 667853
(Reporter)

Comment 51

6 years ago
Comment on attachment 543566 [details] [diff] [review]
DON'T LAND: Re-entrancy test, with change of wording

Moved to bug 668948
Attachment #543566 - Attachment description: One change in wording → DON'T LAND: Re-entrancy test, with change of wording
Attachment #543566 - Attachment is obsolete: true
(Reporter)

Comment 52

6 years ago
Created attachment 543572 [details] [diff] [review]
interdiff of changes to main patch
(Reporter)

Comment 53

6 years ago
Created attachment 543574 [details] [diff] [review]
Remote Websockets v4

Looks good.  The level of error/state checking is less in the e10s code than the mainline, but that looks like a good thing--the nsWebsocket.cpp code does enough checking that many of the things nsWebSocketHandler.cpp is checking for are not possible.  Should cleanup all that logic at some point.  Added some of those checks in e10s in form of assertions.

Running through try one last time, then will land.
Attachment #540609 - Attachment is obsolete: true
Attachment #543574 - Flags: review+
Attachment #540609 - Flags: review?(jduell.mcbugs)
(Reporter)

Comment 54

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/4a894d4dc4da

I'm landing the main patch separately from the SpecialPowers patch, as with it I'm occasionally seeing 

 SpecialPowers is not defined at 
 http://mochi.test:8888/tests/Harness_sanity/file_SpecialPowersFrame1.html:11

mainly only on windows debug.  I don't see any reason why it could be causing that, but just in case..
Whiteboard: [inbound]
(Reporter)

Comment 55

6 years ago
Comment on attachment 535998 [details] [diff] [review]
Update websocket test to use SpecialPowers.

Special powers patch landed as

http://hg.mozilla.org/integration/mozilla-inbound/rev/7a747adc8303
(Assignee)

Comment 56

6 years ago
Adding dev-doc-needed for the SpecialPowers API addition.
Keywords: dev-doc-needed
http://hg.mozilla.org/mozilla-central/rev/4a894d4dc4da
http://hg.mozilla.org/mozilla-central/rev/7a747adc8303
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(In reply to comment #55)
> Comment on attachment 535998 [details] [diff] [review] [review]
> Update websocket test to use SpecialPowers.
> 
> Special powers patch landed as
> 
> http://hg.mozilla.org/integration/mozilla-inbound/rev/7a747adc8303

Please check your commits (hg out) before pushing stuff.
Whiteboard: [inbound]
Blocks: 669819
Depends on: 668950
tracking-fennec: 7+ → 8+
status-firefox8: --- → fixed
tracking-fennec: 8+ → 7+
status-firefox7: --- → fixed
status-firefox8: fixed → ---

Comment 59

6 years ago
I tried http://websockets.org/echo.html and http://mrdoob.com/projects/multiuserpad with browser.tabs.remote=true and the pages work. With browser.tabs.remote=false they don't work, is this the expected result?

Mozilla /5.0 (Android;Linux armv7l;rv:7.0a2) Gecko/20110804 Firefox/7.0a2 Fennec/7.0a2
Device: LG Opyimus 2X
From a developer standpoint, is the key issue here that WebSockets now work on mobile?
(Assignee)

Comment 61

6 years ago
Yes. The problem with testing this is that there are not many public tests that exercise the more recent websocket protocol, plus virtually no tests make use of the prefixed version.
OK, the compatibility tables for WebSockets have been updated.
Keywords: dev-doc-needed → dev-doc-complete
Note that there are virtually no demos online on the web that work in Mozilla, because WebSocket was prefixed with "Moz" in bug 659324.
It turns out there is one site that supports MozWebSocket, that is http://websocketstest.com/ . I used that for a litmus test.
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=26364
Flags: in-litmus+

Comment 65

6 years ago
Mozilla /5.0 (Android;Linux armv7l;rv:7.0) Gecko/20110830 Firefox/7.0 Fennec/7.0
Device: Motorola DROID2

Verified site: http://websocketstest.com/ and every thing was reported as working on that page, except for the 'HTTP Proxy' part.

Marking bug as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.