Last Comment Bug 473841 - "ASSERTION: NULL mIconRequest! Multiple calls to OnStopRequest()?"
: "ASSERTION: NULL mIconRequest! Multiple calls to OnStopRequest()?"
Status: RESOLVED FIXED
: assertion, intermittent-failure
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla6
Assigned To: Joe Drew (not getting mail)
:
: Markus Stange [:mstange]
Mentors:
Depends on:
Blocks: 635796 438871 593278 598577 610225 613708
  Show dependency treegraph
 
Reported: 2009-01-15 15:31 PST by Jesse Ruderman
Modified: 2012-11-25 19:31 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
?


Attachments
remove assert, add condition (791 bytes, patch)
2011-04-29 14:49 PDT, Joe Drew (not getting mail)
jaas: review+
bzbarsky: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2009-01-15 15:31:56 PST
Every once in a while, I get:

###!!! ASSERTION: NULL mIconRequest!  Multiple calls to OnStopRequest()?: 'mIconRequest', file /Users/jruderman/central/widget/src/cocoa/nsMenuItemIconX.mm, line 481

This is a continuation of bug 471101, which turned a null-dereference crash into an assertion (pending figuring out why it happens).  It sounds like figuring out why it happens requires being able to reproduce this bug reliably :/
Comment 1 David Baron :dbaron: ⌚️UTC-10 2009-12-12 13:20:06 PST
This just happened randomly on tinderbox:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1260646724.1260651373.18170.gz
OS X 10.5.2 mozilla-central leak test build on 2009/12/12 11:38:44  
s: moz2-darwin9-slave17

WARNING: NS_ENSURE_TRUE(mMutable) failed: file /builds/slave/mozilla-central-macosx-debug/build/netwerk/base/src/nsSimpleURI.cpp, line 224
###!!! ASSERTION: NULL mIconRequest!  Multiple calls to OnStopRequest()?: 'mIconRequest', file /builds/slave/mozilla-central-macosx-debug/build/widget/src/cocoa/nsMenuItemIconX.mm, line 589
nsMenuItemIconX::OnStopRequest(imgIRequest*, int) (/var/folders/TL/TLg3RrMbFAur2hBCXvCeqk+++TM/-Tmp-//ccP1qjs1.s:256)
imgRequestProxy::OnStopRequest(nsIRequest*, nsISupports*, unsigned int, int) (respcmn.c:43)
imgRequest::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (respcmn.c:43)
ProxyListener::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (respcmn.c:43)
imgCacheValidator::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (respcmn.c:43)
nsJARChannel::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (respcmn.c:43)
nsInputStreamPump::OnStateStop() (respcmn.c:43)
nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (respcmn.c:43)
nsInputStreamReadyEvent::Run() (/var/folders/TL/TLg3RrMbFAur2hBCXvCeqk+++TM/-Tmp-//ccP1qjs1.s:256)
nsThread::ProcessNextEvent(int, int*) (/var/folders/TL/TLg3RrMbFAur2hBCXvCeqk+++TM/-Tmp-//ccP1qjs1.s:256)
NS_ProcessPendingEvents_P(nsIThread*, unsigned int) (/var/folders/TL/TLg3RrMbFAur2hBCXvCeqk+++TM/-Tmp-//ccP1qjs1.s:256)
nsBaseAppShell::NativeEventCallback() (/var/folders/TL/TLg3RrMbFAur2hBCXvCeqk+++TM/-Tmp-//ccP1qjs1.s:256)
nsAppShell::ProcessGeckoEvents(void*) (/var/folders/TL/TLg3RrMbFAur2hBCXvCeqk+++TM/-Tmp-//ccP1qjs1.s:256)
CFRunLoopRunSpecific (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation)
CFRunLoopRunInMode (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation)
RunCurrentEventLoopInMode (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox)
ReceiveNextEventCommon (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox)
BlockUntilNextEventMatchingListInMode (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox)
_DPSNextEvent (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit)
-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit)
-[NSApplication run] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit)
nsAppShell::Run() (/var/folders/TL/TLg3RrMbFAur2hBCXvCeqk+++TM/-Tmp-//ccP1qjs1.s:256)
nsAppStartup::Run() (respcmn.c:43)
XRE_main (respcmn.c:43)
main (/builds/slave/mozilla-central-macosx-debug/build/xpcom/glue/nsComponentManagerUtils.cpp:)
Comment 4 Serge Gautherie (:sgautherie) 2010-10-08 11:51:52 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1286562276.1286562713.32024.gz&fulltext=1
OS X 10.5 comm-central-trunk debug test crashtest on 2010/10/08 11:24:36
{
REFTEST TEST-START | file:///builds/slave/comm-central-trunk-macosx-debug-unittest-crashtest/build/reftest/tests/content/base/crashtests/231475-1.html

###!!! ASSERTION: NULL mIconRequest!  Multiple calls to OnStopRequest()?: 'mIconRequest', file /builds/slave/comm-central-trunk-macosx-debug/build/mozilla/widget/src/cocoa/nsMenuItemIconX.mm, line 549
nsMenuItemIconX::OnStopRequest [widget/src/cocoa/nsMenuItemIconX.mm:550]
imgRequestProxy::OnStopRequest [modules/libpr0n/src/imgRequestProxy.cpp:710]
imgStatusTracker::SendStopRequest [modules/libpr0n/src/imgStatusTracker.cpp:524]
imgRequest::OnStopRequest [modules/libpr0n/src/imgRequest.cpp:959]
ProxyListener::OnStopRequest [modules/libpr0n/src/imgLoader.cpp:2008]
imgCacheValidator::OnStopRequest [modules/libpr0n/src/imgLoader.cpp:2164]
nsJARChannel::OnStopRequest [modules/libjar/nsJARChannel.cpp:907]
nsInputStreamPump::OnStateStop [netwerk/base/src/nsInputStreamPump.cpp:579]
nsInputStreamPump::OnInputStreamReady [netwerk/base/src/nsInputStreamPump.cpp:403]
nsInputStreamReadyEvent::Run [xpcom/io/nsStreamUtils.cpp:113]
nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:547]
NS_ProcessPendingEvents_P [nsThreadUtils.cpp:200]
nsBaseAppShell::NativeEventCallback [widget/src/xpwidgets/nsBaseAppShell.cpp:132]
nsAppShell::ProcessGeckoEvents [widget/src/cocoa/nsAppShell.mm:395]
CoreFoundation + 0x733c5
CoreFoundation + 0x73aa8
HIToolbox + 0x302ac
HIToolbox + 0x300c5
HIToolbox + 0x2ff39
AppKit + 0x406d5
AppKit + 0x3ff88
AppKit + 0x38f9f
nsAppShell::Run [widget/src/cocoa/nsAppShell.mm:747]
nsAppStartup::Run [toolkit/components/startup/src/nsAppStartup.cpp:191]
XRE_main [toolkit/xre/nsAppRunner.cpp:3670]
main [suite/app/nsSuiteApp.cpp:103]
seamonkey-bin + 0x170e

REFTEST TEST-PASS | file:///builds/slave/comm-central-trunk-macosx-debug-unittest-crashtest/build/reftest/tests/content/base/crashtests/231475-1.html | (LOAD ONLY)
...
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/comm-central-trunk-macosx-debug-unittest-crashtest/build/reftest/tests/content/base/crashtests/231475-1.html | assertion count 1 is more than expected 0 assertions
}
Comment 5 David Baron :dbaron: ⌚️UTC-10 2010-10-13 12:58:23 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1286987397.1286997533.24647.gz
OS X 10.5.2 mozilla-central leak test build on 2010/10/13 09:29:57
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-01 23:42:10 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1288670720.1288679411.11595.gz
Comment 7 Serge Gautherie (:sgautherie) 2010-11-07 08:13:45 PST
"blocking2.0=?":
Though intermittent, this seems to happen frequently enough that it should be reproducible to analyze.
Comment 8 Joe Drew (not getting mail) 2010-11-09 14:37:56 PST
I agree we should investigate it, but doing so doesn't block 2.0.
Comment 13 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-03-18 11:55:52 PDT
Joe, I notice that nsMenuItemIconX::LoadIcon and nsMenuItemIconX::OnStopRequest both call mIconRequest->Cancel instead of the synchronous CancelAndForgetObserver in use elsewhere in the file.  Since Cancel is asynchronous, presumably we would null out mIconRequest after calling it, and we could end up triggering the assertion when the final OnStopRequest comes through.  I'm not sure what the stacks would look like in this case - does the one in comment 1 match up with my idea?
Comment 15 :Ehsan Akhgari 2011-04-14 20:18:53 PDT
(In reply to comment #13)
> Joe, I notice that nsMenuItemIconX::LoadIcon and nsMenuItemIconX::OnStopRequest
> both call mIconRequest->Cancel instead of the synchronous
> CancelAndForgetObserver in use elsewhere in the file.  Since Cancel is
> asynchronous, presumably we would null out mIconRequest after calling it, and
> we could end up triggering the assertion when the final OnStopRequest comes
> through.  I'm not sure what the stacks would look like in this case - does the
> one in comment 1 match up with my idea?

Joe, any ideas?
Comment 17 Joe Drew (not getting mail) 2011-04-20 13:35:30 PDT
(In reply to comment #13)
> Joe, I notice that nsMenuItemIconX::LoadIcon and nsMenuItemIconX::OnStopRequest
> both call mIconRequest->Cancel instead of the synchronous
> CancelAndForgetObserver in use elsewhere in the file.  Since Cancel is
> asynchronous, presumably we would null out mIconRequest after calling it, and
> we could end up triggering the assertion when the final OnStopRequest comes
> through.

To be clear, are you asking whether OnStopRequest could come in after LoadIcon? Yes, that sounds likely. We can't just blindly switch to CancelAndForgetObserver, though, since that's synchronous and thus re-enters ourselves. In this case it might be OK, though - Boris, do you have any ideas?

The only way that I can see this happening is if we have a current request that hasn't started loading, then try to load a separate URI that doesn't load properly (because we know mIconRequest is null).
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2011-04-20 19:10:23 PDT
> Boris, do you have any ideas?

How about we just fix the broken assertion?
Comment 20 Joe Drew (not getting mail) 2011-04-29 14:49:00 PDT
Created attachment 529211 [details] [diff] [review]
remove assert, add condition

Presuming the description in comment 13 and comment 17 is correct, this seems like the right solution. It will ensure that we only care about OnStopRequest calls from our current icon request.
Comment 30 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-05-14 05:11:40 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1305369653.1305373611.21420.gz
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2011-05-16 12:08:50 PDT
Comment on attachment 529211 [details] [diff] [review]
remove assert, add condition

r=me

Note You need to log in before you can comment on or make changes to this bug.