1179255 - (fatal) Assertion failure: aRequest == mCancelable, at netwerk/protocol/websocket/WebSocketChannel.cpp:2766 in WebSocketChannel::OnProxyAvailable, with FoxyProxy Basic installed, after clicking "Get Started" in Firefox Hello menu

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Daniel Holbert [:dholbert]]

Just tried running with my normal everyday web-browsing profile, with a debug build, for the first time in a while. My debug build was built from yesterday's mozilla-central changeset 291614a686f1

While tabs were being restored, I hit this fatal assertion-failure:

Assertion failure: aRequest == mCancelable, at $SRC/netwerk/protocol/websocket/WebSocketChannel.cpp:2766
#01: mozilla::net::WebSocketChannel::OnProxyAvailable(nsICancelable*, nsIChannel*, nsIProxyInfo*, nsresult) ($SRC/netwerk/protocol/websocket/WebSocketChannel.cpp:2766 (discriminator 2))
#02: non-virtual thunk to mozilla::net::WebSocketChannel::OnProxyAvailable(nsICancelable*, nsIChannel*, nsIProxyInfo*, nsresult) ($SRC/netwerk/protocol/websocket/WebSocketChannel.cpp:2757)
#03: nsAsyncResolveRequest::DoCallback() ($SRC/netwerk/base/nsProtocolProxyService.cpp:287)
#04: nsAsyncResolveRequest::Run() ($SRC/netwerk/base/nsProtocolProxyService.cpp:169)
#05: nsThread::ProcessNextEvent(bool, bool*) ($SRC/xpcom/threads/nsThread.cpp:849)
#06: NS_InvokeByIndex ($SRC/xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:174)
#07: CallMethodHelper::Invoke() ($SRC/js/xpconnect/src/XPCWrappedNative.cpp:2080)
#08: CallMethodHelper::Call() ($SRC/js/xpconnect/src/XPCWrappedNative.cpp:1417)
#09: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) ($SRC/js/xpconnect/src/XPCWrappedNative.cpp:1384)
#10: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) ($SRC/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1144)
#11: ??? (???:???)

I happened to catch this in rr (because I'm trying to catch a different bug in rr and tripped over this one first), so I can probably poke around if it's useful.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> I happened to catch this in rr (because I'm trying to catch a different bug
> in rr and tripped over this one first), so I can probably poke around if
> it's useful.

Just kidding; I tripped over another assertion failure in rr itself, when trying to replay the trace. Whelp.

Comment 2

[Daniel Holbert [:dholbert]]

Despite my rr issues, I can hit this (and catch it in gdb) reliably on startup.

I've got it trapped in gdb right now.  This is happening on the very first call to WebSocketChannel::OnProxyAvailable that I ever receive. (I've got a breakpoint in that function, and I can see that this asserted condition is already false the first time my breakpoint is hit.)

From gdb:
> (gdb) p aRequest
> $1 = (nsAsyncResolveRequest *) 0x7f7c142c6790
> (gdb) p mCancelable
> $2 = {
>   mRawPtr = 0x0
> }
> (gdb) p mStopped
> $3 = {
>   <mozilla::detail::AtomicBase<unsigned int, mozilla::MemoryOrdering::SequentiallyConsistent>> = {
>     mValue = {
>       <std::__atomic_base<unsigned int>> = {
>         _M_i = 0
>       }, <No data fields>}
>   }, <No data fields>}

Comment 3

[Patrick McManus [:mcmanus]]

michal is doing websocket maint at the moment.. ni xferred

Comment 4

[Daniel Holbert [:dholbert]]

I actually hit this exact issue several times during tab-restoration.  (if I bypass the assertion in gdb, I end up hitting it again slightly later)

I tried putting a breakpoint in the WebSocketChannel constructor and watching mCancelable.mRawPtr (which starts out null). Its value simply isn't changed between the constructor & this OnProxyAvailable call. So, it's just still null from the constructor.

Perhaps we mean to be asserting !mCancelable || aRequest == mCancelable, instead of simply asserting aRequest == mCancelable?

Comment 5

[Daniel Holbert [:dholbert]]

(I have yet to see my breakpoint in OnProxyAvailable hit *without* a null mCancelable pointer.  AFAICT, this assertion fails every time this function is called, in my local browsing session at least.)

Comment 6

[Michal Novotny [:michal]]

Do you have reliable STR? URL of the failing channel and your proxy setting could be enough.


(In reply to Daniel Holbert [:dholbert] from comment #4)
> I tried putting a breakpoint in the WebSocketChannel constructor and
> watching mCancelable.mRawPtr (which starts out null). Its value simply isn't
> changed between the constructor & this OnProxyAvailable call. So, it's just
> still null from the constructor.

This is strange. OnProxyAvailable is called only if nsIProtocolProxyService::AsyncResolve succeeds and in this case we must have a valid mCancelable. Could you please set a breakpoint in WebSocketChannel::ApplyForAdmission and see why nsIProtocolProxyService::AsyncResolve doesn't correctly set mCancelable? From what I see in the code nsProtocolProxyService calls the callback iff it also returns cancelable result.


> Perhaps we mean to be asserting !mCancelable || aRequest == mCancelable,
> instead of simply asserting aRequest == mCancelable?

No, as I wrote above, there always should be mCancelable.

Comment 7

[Daniel Holbert [:dholbert]]

OK, I have reliable STR -- there's an interaction between FoxyProxy and signed-in (or signing-in) Firefox Hello.

STR (performed in a debug build of Firefox)
===========================================
 1. Install FoxyProxy Basic from here:
     https://addons.mozilla.org/en-US/firefox/addon/foxyproxy-basic/
    (ignore notes about it not having been proven compatible w/ latest Firefox version)

 2. Restart Firefox to complete installation.

 3. Click "Firefox Hello" icon on toolbar.
 4. Click "Get Started"

ACTUAL RESULTS:
Assertion failure: aRequest == mCancelable, at $SRC/netwerk/protocol/websocket/WebSocketChannel.cpp:2766

#01: nsCOMPtr<nsIProtocolProxyCallback>::assign_assuming_AddRef(nsIProtocolProxyCallback*) ($OBJ/netwerk/base/../../dist/include/nsCOMPtr.h:370)
#02: nsAsyncResolveRequest::Run() ($SRC/netwerk/base/nsProtocolProxyService.cpp:169)
#03: nsThread::ProcessNextEvent(bool, bool*) ($SRC/xpcom/threads/nsThread.cpp:848)

Comment 8

[Daniel Holbert [:dholbert]]

This bug is a startup-abort for me, in my main browsing profile, if I have FoxyProxy Basic installed and also have the pref "loop.fxa_oauth.tokendata" in prefs.js. (Its value is an auth token of some sort.)

But I can also trigger it, starting with a fresh profile, with the steps in comment 7.

Comment 9

[Michal Novotny [:michal]]

FoxyProxy addon reimplements ProtocolProxyService but its implementation of asyncResolve() doesn't return nsICancelable object. Description in nsIProtocolProxyService.idl doesn't mention that null can be returned, so it seems to me like a bug in FoxyProxy. Should we be less strict to be compatible with buggy addon?

Comment 10

[Daniel Holbert [:dholbert]]

Two things:
 (1) The FoxyProxy devs are pretty responsive (they're JesperHansen & ericjung on irc.m.o), and will likely be interested in fixing the add-on bug here.

 (2) Regardless of FoxyProxy bugs / progress -- at the very least, we should soften this assertion to be nonfatal. Fatal assertions are severe and prevent people from proceeding to debug the thing they're actually intended to debug.  Hence, IMO, they should only be used in cases where we're really about to crash or do something really bad.  (That doesn't seem to be the case here, as my opt build has been gleefully sailing past this code without issue, for the indefinite past.)  See also http://robert.ocallahan.org/2011/12/case-for-non-fatal-assertions.html

Comment 11

[Daniel Holbert [:dholbert]]

(following up on (2): in my case, this fatal assertion manifested as a start-up-abort last week, when I was trying to reproduce a completely unrelated issue. On the one hand, the start-up-abort resulted in me being more likely to report this bug [as compared to an alternate universe where this was just a NS_ASSERTION], which is good. But on the other hand, it blocked my progress for a while, as I tried to figure out what was going on and then hacked around the abort so that I could proceed [only to hit a completely different fatal startup-assertion in the JS engine, which I filed separately and also had to work past before I could actually try to reproduce the thing I was actually interested in :-/])

Comment 12

[Jesper Hansen]

Fixing in next FP release.
Anything you want to do on your end?

Comment 13

[Patrick McManus [:mcmanus]]

(In reply to Jesper Hansen from comment #12)
> Fixing in next FP release.

Thanks. definitely good to have a cancelable

> Anything you want to do on your end?

I think we should update our debug assert too

Comment 14

[Patrick McManus [:mcmanus]]

(In reply to Michal Novotny (:michal) from comment #9)
>Should we be less strict to be
> compatible with buggy addon?

I think in this case we should tolerate it.. MOZ_ASSERT(!mCancelable || (aRequest == mCancelable)) ?

Comment 15

[Jesper Hansen]

This might have triggered because we found out that asyncResolve doesn't make the call to the addon asynchronous. That is covered in bug 1152332

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Patrick McManus [:mcmanus] from comment #14)
> (In reply to Michal Novotny (:michal) from comment #9)
> >Should we be less strict to be
> > compatible with buggy addon?
> 
> I think in this case we should tolerate it.. MOZ_ASSERT(!mCancelable || (aRequest == mCancelable)) ?

That sounds good to me! (I made the same suggestion in comment 4. :))

If we're tolerating a null value but really do *want* addons/etc. to pass in non-null values here, maybe we should combine this with a nonfatal NS_ASSERTION() to null-check mCancelable?  (so that we won't abort, but it's at least discoverable that something's wrong)

Comment 17

[Michal Novotny [:michal]]

Attachment: fix

Comment 18

[Pulsebot]

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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d4e36668617e