771318 - Heap-use-after-free in nsWSAdmissionManager::OnStopSession

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Abhishek Arya]

Reproduces on trunk.
20120705010422
http://hg.mozilla.org/mozilla-central/rev/675f55c4310c

Flaky crash on mutation of testcase in bug 603750. Will update bug if i get a reliable testcase.

=================================================================
==2854== ERROR: AddressSanitizer heap-use-after-free on address 0x7fb1da389990 at pc 0x7fb1fc2ef86c bp 0x7fffcc052b70 sp 0x7fffcc052b68
READ of size 4 at 0x7fb1da389990 thread T0
    #0 0x7fb1fc2ef86c in mozilla::net::nsWSAdmissionManager::OnStopSession(mozilla::net::WebSocketChannel*, unsigned int) netwerk/protocol/websocket/WebSocketChannel.cpp:376
    #1 0x7fb1feeffda4 in NS_ProcessNextEvent_P(nsIThread*, bool) obj-firefox/xpcom/build/nsThreadUtils.cpp:217
    #2 0x7fb1fec2a9f8 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:82
    #3 0x7fb1ff04202c in ~AutoRunState ipc/chromium/src/base/message_loop.cc:495
    #4 0x7fb1fe977dde in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:165
    #5 0x7fb1fc00f4b1 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3863
    #6 0x7fb1fc01050b in XRE_main toolkit/xre/nsAppRunner.cpp:3939
    #7 0x408e83 in do_main(int, char**) browser/app/nsBrowserApp.cpp:160
    #8 0x7fb204793c4d in __libc_start_main /build/buildd/eglibc-2.11.1/csu/libc-start.c:258
0x7fb1da389990 is located 272 bytes inside of 1840-byte region [0x7fb1da389880,0x7fb1da389fb0)
freed by thread T0 here:
    #0 0x424442 in free 
    #1 0x7fb1fc2d8dfe in mozilla::net::WebSocketChannel::Release() netwerk/protocol/websocket/WebSocketChannel.cpp:56
    #2 0x7fb1fce70999 in nsWebSocket::Init(nsIPrincipal*, nsIScriptContext*, nsPIDOMWindow*, nsAString_internal const&, nsTArray<nsString, nsTArrayDefaultAllocator>&) content/base/src/nsWebSocket.cpp:1585
    #3 0x7fb1fce6a2db in nsWebSocket::Initialize(nsISupports*, JSContext*, JSObject*, unsigned int, JS::Value*) content/base/src/nsWebSocket.cpp:694
    #4 0x7fb1fd5449ed in nsCOMPtr<nsIWeakReference>::operator nsIWeakReference*() const dom/base/nsDOMClassInfo.cpp:5731
    #5 0x7fb1fd5665b6 in non-virtual thunk to nsDOMConstructorSH::Construct(nsIXPConnectWrappedNative*, JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*, bool*) dom/base/nsDOMClassInfo.cpp:10642
    #6 0x7fb1ffb4494b in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), js::CallArgs const&) js/src/jscntxtinlines.h:400
    #7 0x7fb1ffc0b1a7 in js::IndirectProxyHandler::construct(JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*) js/src/jsproxy.cpp:454
    #8 0x7fb1ffd1e78b in js::DirectWrapper::construct(JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*) js/src/jswrapper.cpp:311
    #9 0x7fb1ffd1ea8a in non-virtual thunk to js::CrossCompartmentWrapper::construct(JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*) 
    #10 0x7fb1ffb4494b in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), js::CallArgs const&) js/src/jscntxtinlines.h:400
    #11 0x7fb1ffb189f0 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) js/src/jsinterp.cpp:297
    #12 0x7fb1ffb435d5 in js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:348
    #13 0x7fb1ffb4466c in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) js/src/jsinterp.h:119
    #14 0x7fb1ffa10f78 in JS_CallFunctionValue js/src/jsapi.cpp:5535
    #15 0x7fb1fdf7e1d4 in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) js/xpconnect/src/XPCWrappedJSClass.cpp:1436
    #16 0x7fb1fdf70355 in nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) js/xpconnect/src/XPCWrappedJS.cpp:580
    #17 0x7fb1feff61a2 in PrepareAndDispatch xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:121
    #18 0x7fb1feff51e3 in SharedStub xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:0
    #19 0x7fb1fcfd539e in nsRefPtr<nsIDOMEventListener>::operator nsIDOMEventListener*() const content/events/src/nsEventListenerManager.cpp:818
    #20 0x7fb1fd03579d in nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int, bool, nsCxPusher*) content/events/src/nsEventListenerManager.h:143
previously allocated by thread T0 here:
    #0 0x424502 in __interceptor_malloc 
    #1 0x7fb201c721a9 in moz_xmalloc memory/mozalloc/mozalloc.cpp:54
==2854== ABORTING
Stats: 137M malloced (165M for red zones) by 390620 calls
Stats: 29M realloced by 14993 calls
Stats: 92M freed by 149324 calls
Stats: 0M really freed by 0 calls
Stats: 340M (87089 full pages) mmaped in 85 calls
  mmaps   by size class: 8:344043; 9:40955; 10:16380; 11:12282; 12:3072; 13:2560; 14:1536; 15:384; 16:640; 17:96; 18:160; 19:40; 20:16;
  mallocs by size class: 8:322499; 9:38213; 10:11968; 11:11008; 12:2251; 13:2092; 14:1423; 15:297; 16:597; 17:69; 18:151; 19:39; 20:13;
  frees   by size class: 8:103752; 9:24604; 10:8113; 11:8045; 12:1486; 13:1215; 14:1201; 15:244; 16:531; 17:55; 18:30; 19:37; 20:11;
  rfrees  by size class:
Stats: malloc large: 272 small slow: 1874
Shadow byte and word:
  0x1ff63b471332: fd
  0x1ff63b471330: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1ff63b471310: fd fd fd fd fd fd fd fd
  0x1ff63b471318: fd fd fd fd fd fd fd fd
  0x1ff63b471320: fd fd fd fd fd fd fd fd
  0x1ff63b471328: fd fd fd fd fd fd fd fd
=>0x1ff63b471330: fd fd fd fd fd fd fd fd
  0x1ff63b471338: fd fd fd fd fd fd fd fd
  0x1ff63b471340: fd fd fd fd fd fd fd fd
  0x1ff63b471348: fd fd fd fd fd fd fd fd
  0x1ff63b471350: fd fd fd fd fd fd fd fd

Comment 1

[Jason Duell]

Thanks--I've got a handle on this, I think.  Patch coming soon.

Comment 2

[Abhishek Arya]

Attachment: Testcase

To make your task even more easier, here is a reproducible testcase and fully symbolized stack.

=================================================================
==21305== ERROR: AddressSanitizer heap-use-after-free on address 0x7fca37a7d190 at pc 0x7fca6354cb1a bp 0x7fffe523ccd0 sp 0x7fffe523ccc8
READ of size 4 at 0x7fca37a7d190 thread T0
    #0 0x7fca6354cb1a in mozilla::net::nsWSAdmissionManager::OnStopSession(mozilla::net::WebSocketChannel*, unsigned int) src/netwerk/protocol/websocket/WebSocketChannel.cpp:376
    #1 0x7fca6354a555 in mozilla::net::CallOnStop::Run() src/netwerk/protocol/websocket/WebSocketChannel.cpp:538
    #2 0x7fca6f0c24ed in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:625
    #3 0x7fca6ed512fd in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:217
    #4 0x7fca6ddf34f6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
    #5 0x7fca6f37680a in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:209
    #6 0x7fca6f376653 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:202
    #7 0x7fca6f376538 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:176
    #8 0x7fca6d32c49e in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:165
    #9 0x7fca6bf772f8 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:256
    #10 0x7fca62932ea7 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3786
    #11 0x7fca62939862 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3863
    #12 0x7fca6293cd1b in XRE_main src/toolkit/xre/nsAppRunner.cpp:3939
    #13 0x40c28f in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:160
    #14 0x409cbd in main src/browser/app/nsBrowserApp.cpp:330
    #15 0x7fca7d3d5c4d in ?? ??:0
0x7fca37a7d190 is located 272 bytes inside of 1840-byte region [0x7fca37a7d080,0x7fca37a7d7b0)
freed by thread T0 here:
    #0 0x4a4392 in free ??:0
    #1 0x7fca7a2635c3 in moz_free src/memory/mozalloc/mozalloc.cpp:49
    #2 0x7fca6350bd96 in ~WebSocketChannel src/netwerk/protocol/websocket/WebSocketChannel.cpp:1014
    #3 0x7fca635053d2 in mozilla::net::WebSocketChannel::Release() src/netwerk/protocol/websocket/WebSocketChannel.cpp:56
    #4 0x7fca6ed07bb0 in ~nsCOMPtr_base src/objdir-ff-asan-sym/xpcom/build/nsCOMPtr.cpp:48
    #5 0x7fca63567d46 in nsCOMPtr<nsIWebSocketChannel>::~nsCOMPtr() src/../../../dist/include/nsCOMPtr.h:442
    #6 0x7fca63567c33 in nsCOMPtr<nsIWebSocketChannel>::~nsCOMPtr() src/../../../dist/include/nsCOMPtr.h:442
    #7 0x7fca66066d27 in nsWebSocket::EstablishConnection() src/content/base/src/nsWebSocket.cpp:769
    #8 0x7fca6607d7fb in nsWebSocket::Init(nsIPrincipal*, nsIScriptContext*, nsPIDOMWindow*, nsAString_internal const&, nsTArray<nsString, nsTArrayDefaultAllocator>&) src/content/base/src/nsWebSocket.cpp:1587
    #9 0x7fca6606381e in nsWebSocket::Initialize(nsISupports*, JSContext*, JSObject*, unsigned int, JS::Value*) src/content/base/src/nsWebSocket.cpp:694
    #10 0x7fca6606574f in non-virtual thunk to nsWebSocket::Initialize(nsISupports*, JSContext*, JSObject*, unsigned int, JS::Value*) src/modules/zlib/src/inffast.c:0
    #11 0x7fca680deda1 in BaseStubConstructor(nsIWeakReference*, nsGlobalNameStruct const*, JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*) src/dom/base/nsDOMClassInfo.cpp:5735
    #12 0x7fca680dc91a in nsDOMConstructor::Construct(nsIXPConnectWrappedNative*, JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*, bool*) src/dom/base/nsDOMClassInfo.cpp:6235
    #13 0x7fca6818194d in nsDOMConstructorSH::Construct(nsIXPConnectWrappedNative*, JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*, bool*) src/dom/base/nsDOMClassInfo.cpp:10648
    #14 0x7fca68181b9f in non-virtual thunk to nsDOMConstructorSH::Construct(nsIXPConnectWrappedNative*, JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*, bool*) src/modules/zlib/src/inffast.c:0
    #15 0x7fca6ab0e35b in XPC_WN_Helper_Construct(JSContext*, unsigned int, JS::Value*) src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:983
    #16 0x7fca744f161c in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:400
    #17 0x7fca73d2cf98 in js::InvokeConstructor(JSContext*, js::InvokeArgsGuard&) src/js/src/jsinterp.h:153
    #18 0x7fca744f3da3 in js::InvokeConstructor(JSContext*, JS::Value const&, unsigned int, JS::Value*, JS::Value*) src/js/src/jsinterp.cpp:438
    #19 0x7fca7492d120 in js::IndirectProxyHandler::construct(JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*) src/js/src/jsproxy.cpp:454
    #20 0x7fca750cc37e in js::DirectWrapper::construct(JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*) src/js/src/jswrapper.cpp:311
    #21 0x7fca750e0f77 in js::CrossCompartmentWrapper::construct(JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*) src/js/src/jswrapper.cpp:720
    #22 0x7fca750e160d in non-virtual thunk to js::CrossCompartmentWrapper::construct(JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*) ??:0
    #23 0x7fca74961deb in js::Proxy::construct(JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*) src/js/src/jsproxy.cpp:1142
    #24 0x7fca74979b44 in proxy_Construct(JSContext*, unsigned int, JS::Value*) src/js/src/jsproxy.cpp:1665
    #25 0x7fca744f161c in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:400
    #26 0x7fca7445be42 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2462
    #27 0x7fca743e07b7 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) src/js/src/jsinterp.cpp:299
    #28 0x7fca744e963f in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) src/js/src/jsinterp.cpp:350
    #29 0x7fca73e7c560 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) src/js/src/jsinterp.h:119
previously allocated by thread T0 here:
    #0 0x4a4452 in __interceptor_malloc ??:0
    #1 0x7fca7a263717 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:54
    #2 0x7fca62a2e270 in mozilla::net::WebSocketChannelConstructor(bool) src/netwerk/build/nsNetModule.cpp:273
    #3 0x7fca62a2b49f in mozilla::net::WebSocketChannelConstructor(nsISupports*, nsID const&, void**) src/netwerk/build/nsNetModule.cpp:298
    #4 0x7fca6ed8d662 in mozilla::GenericFactory::CreateInstance(nsISupports*, nsID const&, void**) src/objdir-ff-asan-sym/xpcom/build/GenericFactory.cpp:16
    #5 0x7fca6f06b551 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) src/xpcom/components/nsComponentManager.cpp:1031
    #6 0x7fca6ed149aa in CallCreateInstance(char const*, nsISupports*, nsID const&, void**) src/objdir-ff-asan-sym/xpcom/build/nsComponentManagerUtils.cpp:135
    #7 0x7fca6ed15e2a in nsCreateInstanceByContractID::operator()(nsID const&, void**) const src/objdir-ff-asan-sym/xpcom/build/nsComponentManagerUtils.cpp:178
    #8 0x7fca6ed0936d in nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&, nsID const&) src/objdir-ff-asan-sym/xpcom/build/nsCOMPtr.cpp:117
    #9 0x7fca63562607 in nsCOMPtr<nsIWebSocketChannel>::operator=(nsCOMPtr_helper const&) src/../../../dist/include/nsCOMPtr.h:686
    #10 0x7fca66065b8b in nsWebSocket::EstablishConnection() src/content/base/src/nsWebSocket.cpp:734
    #11 0x7fca6607d7fb in nsWebSocket::Init(nsIPrincipal*, nsIScriptContext*, nsPIDOMWindow*, nsAString_internal const&, nsTArray<nsString, nsTArrayDefaultAllocator>&) src/content/base/src/nsWebSocket.cpp:1587
    #12 0x7fca6606381e in nsWebSocket::Initialize(nsISupports*, JSContext*, JSObject*, unsigned int, JS::Value*) src/content/base/src/nsWebSocket.cpp:694
    #13 0x7fca6606574f in non-virtual thunk to nsWebSocket::Initialize(nsISupports*, JSContext*, JSObject*, unsigned int, JS::Value*) src/modules/zlib/src/inffast.c:0
    #14 0x7fca680deda1 in BaseStubConstructor(nsIWeakReference*, nsGlobalNameStruct const*, JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*) src/dom/base/nsDOMClassInfo.cpp:5735
    #15 0x7fca680dc91a in nsDOMConstructor::Construct(nsIXPConnectWrappedNative*, JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*, bool*) src/dom/base/nsDOMClassInfo.cpp:6235
    #16 0x7fca6818194d in nsDOMConstructorSH::Construct(nsIXPConnectWrappedNative*, JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*, bool*) src/dom/base/nsDOMClassInfo.cpp:10648
    #17 0x7fca68181b9f in non-virtual thunk to nsDOMConstructorSH::Construct(nsIXPConnectWrappedNative*, JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*, bool*) src/modules/zlib/src/inffast.c:0
    #18 0x7fca6ab0e35b in XPC_WN_Helper_Construct(JSContext*, unsigned int, JS::Value*) src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:983
    #19 0x7fca744f161c in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:400
    #20 0x7fca73d2cf98 in js::InvokeConstructor(JSContext*, js::InvokeArgsGuard&) src/js/src/jsinterp.h:153
    #21 0x7fca744f3da3 in js::InvokeConstructor(JSContext*, JS::Value const&, unsigned int, JS::Value*, JS::Value*) src/js/src/jsinterp.cpp:438
    #22 0x7fca7492d120 in js::IndirectProxyHandler::construct(JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*) src/js/src/jsproxy.cpp:454
    #23 0x7fca750cc37e in js::DirectWrapper::construct(JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*) src/js/src/jswrapper.cpp:311
    #24 0x7fca750e0f77 in js::CrossCompartmentWrapper::construct(JSContext*, JSObject*, unsigned int, JS::Value*, JS::Value*) src/js/src/jswrapper.cpp:720
==21305== ABORTING
Stats: 151M malloced (165M for red zones) by 355584 calls
Stats: 41M realloced by 19483 calls
Stats: 111M freed by 228343 calls
Stats: 0M really freed by 0 calls
Stats: 344M (88113 full pages) mmaped in 86 calls
  mmaps   by size class: 8:278511; 9:49146; 10:20475; 11:18423; 12:3072; 13:2048; 14:1536; 15:384; 16:576; 17:128; 18:176; 19:40; 20:16;
  mallocs by size class: 8:267900; 9:47210; 10:16432; 11:17144; 12:2425; 13:1840; 14:1415; 15:334; 16:555; 17:112; 18:164; 19:40; 20:13;
  frees   by size class: 8:159928; 9:36853; 10:13017; 11:13915; 12:1508; 13:947; 14:1225; 15:278; 16:468; 17:98; 18:58; 19:38; 20:10;
  rfrees  by size class:
Stats: malloc large: 329 small slow: 1863
Shadow byte and word:
  0x1ff946f4fa32: fd
  0x1ff946f4fa30: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1ff946f4fa10: fd fd fd fd fd fd fd fd
  0x1ff946f4fa18: fd fd fd fd fd fd fd fd
  0x1ff946f4fa20: fd fd fd fd fd fd fd fd
  0x1ff946f4fa28: fd fd fd fd fd fd fd fd
=>0x1ff946f4fa30: fd fd fd fd fd fd fd fd
  0x1ff946f4fa38: fd fd fd fd fd fd fd fd
  0x1ff946f4fa40: fd fd fd fd fd fd fd fd
  0x1ff946f4fa48: fd fd fd fd fd fd fd fd
  0x1ff946f4fa50: fd fd fd fd fd fd fd fd

Comment 3

[Jason Duell]

Attachment: cleanup patch #1

Patrick: if you want I can file these cleanups as a separate bug, but seems harmless enough to do them here.

Comment 4

[Jason Duell]

Attachment: cleanup patch #2: bool removal

Gets rid of a whole lots of bools being passed around, all in order to issue one LOG message (which is misleading anyway since failure is a 3rd case).

Comment 5

[Jason Duell]

Alas, my main patch (#3) isn't passing Abhishek's test.  Might as well hold off reviewing everything I've posted so far.

Comment 6

[Jason Duell]

Attachment: necko fix v1

So the problem here is that we call StopSession() in the WebSocketChannel destructor, which schedules an event (CallOnStop) that is trying to access the channel *after* it's been destroyed.

This can only happen during early failure when WebSocketChannel::AsyncOpen doesn't succeed normally:  otherwise we've already called StopSession by destructor time.   The event used to be guarded by mListener being != null, so we never hit it, but that changed with my reconnect delay patch.

The test code sets a timer with timeout=0 that reloads the page, so we're creating and then cancelling the pair of websockets over and over.  In some cases this results in the LoadGroup->AddRequest() call in nsWebSocket::EstablishCxn to fail (when the loadGroup is marked as cancelled), which gives us the early WebSocketChannel::AsyncOpen failure that was causing the crash. 

I suspect the Loadgroup being cancelled is some sort of race/bug in the DocShell code: logically when onload fires and the websockets are created, the loadgroup should not yet be canceled, as the timer event is created after that, and even with timeout==0 ought to not run before both websockets have finished calling EstablishCxn.  Weird.

In any case, I've fixed websockets so this will no longer cause a crash.  The test code now winds up in a fast loop as the page reloads over and over and tries to connect new websockets.  I was worried this defeated the new websocket reconnect delay algorithm, but it doesn't:  websockets canceled by page navigation/reloads should not count as failed connections, and I'm inclined not to worry about the rare early failure case we're seeing here (it certainly won't affect the "server hit by thundering herd of clients" problem that the reconnect algorithm was designed for, since we never even start the TCP connection).

On the fix: I've disliked the call to StopSession in ~WebSocketChannel for a long time--I suspected it was superflous, and it makes the code harder to understand.  I've audited it and the one thing that needs to change to get rid of it is that we no longer call OnStop() on the listener if AsyncOpen failed--which we're not supposed to do anyway, but nsWebSocketChannel was relying on it.   I've changed that in the patch for smaug.

I've added some assertions (note that MOZ_ASSERT aborts if debug, so these are just as good--and shorter to type--than NS_ABORT_IF_FALSE).

I'd add the test case as a mochitest but it depends on the page being reload()ed and I'm not at all sure that's possible within mochitest.

Comment 7

[Jason Duell]

Attachment: DOM-side fix, v1

Pretty simple--we need to handle calling onerror/onclose in the DOM now if WebSocketChannel::AsyncOpen fails.  This now follows the nsIWebSocket{Channel|Listener} interface: w/o the necko fix here we were accidentally having the channel call OnStop even if asyncOpen failed.

Comment 8

[Jason Duell]

Oh, and thanks for the test case, Abhishek.  It was *really* helpful.

Comment 9

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

Jason, who is supposed to review those Necko patches?

Comment 10

[Jason Duell]

I'm a victim again of bugzilla silently erasing my patch reviewer choice if they're not already cc'd on the bug :)

Comment 11

[Jason Duell]

bz: Do you think the seemingly-cancelled-too-early loadgroup in comment 6 (just look @ paragraph 3 + 4) is a DocShell bug?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> bz: Do you think the seemingly-cancelled-too-early loadgroup in comment 6 (just look @
> paragraph 3 + 4) is a DocShell bug?

When a loadgroup is canceled, it will end up in the docshell "stop loading" code, which can fire onload, depending on how it was canceled.

In this case, the reload() call will trigger onload, via canceling the loadgroup.

Comment 13

[Jason Duell]

The control path is a little convoluted:  assuming 'DOMContentLoaded' always happens before 'load' (right?), we're running code in DOMContentLoaded that schedules a zero-timeout timer, that when fired calls location.reload.  Whenever 'load' happens we create the websockets.   In most cases the loadGroup isn't cancelled yet: in some it is.  I'm going to chalk it up to it being correct for the load/timer events to race, unless someone tells me otherwise.

Comment 14

[Boris Zbarsky [:bzbarsky]]

DOMContentLoaded always fires before onload.

Racing between a 0-timeout timer from DOMContentLoaded and onload is totally expected, yes.

Comment 15

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0efe9a4f4da

https://hg.mozilla.org/integration/mozilla-inbound/rev/d51590af8299

Comment 16

[Jason Duell]

https://hg.mozilla.org/mozilla-central/rev/d51590af8299
https://hg.mozilla.org/mozilla-central/rev/a0efe9a4f4da

Comment 17

[Daniel Veditz [:dveditz]]

Since no one's claiming this is a recent regression I'm assuming we need this on the ESR-10 branch and in Beta (Fx15)

Comment 18

[Jason Duell]

Sorry, the regression was caused by bug 711793, which is also FF16, so don't need esr/Fx15 tracking.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

This was found and fixed within the Firefox 16 release cycle but by a security researcher. Dan, do we want an advisory for this or not since it was never in a release?

Comment 20

[Al Billings [:abillings - ex-MoCo]]

I'm going to go with including an advisory for this.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Attachment: Bounty Awarded $3000 [paid]