25160 - rapid click of "Find" button on composer crashes

Status: VERIFIED WORKSFORME

(Core :: DOM: Editor, defect, P1)

Description

[Andrew Niese]

see summary. This is on the M13 Win32 build.

1) open composer/editor
2) click on the "Find" button several times rapidly.
3) crash

see talkback incident ID TB4497831E

Comment 1

[rubydoo123]

assigning this one to smfr for debugging

Comment 2

[Simon Fraser [no longer active]]

Here's the stack:

nsXPCWrappedNativeClass::CallWrappedMethod  [d:\builds\seamonkey\mozilla\js\src\
xpconnect\src\xpcwrappednativeclass.cpp, line 910]
WrappedNative_GetProperty                   [d:\builds\seamonkey\mozilla\js\src\
xpconnect\src\xpcwrappednativejsops.cpp, line 242]
js_Interpret                                [d:\builds\seamonkey\mozilla\js\src\
jsinterp.c, line 2220]
js_Invoke                                   [d:\builds\seamonkey\mozilla\js\src\
jsinterp.c, line 687]
js_Interpret                                [d:\builds\seamonkey\mozilla\js\src\
jsinterp.c, line 2263]
js_Invoke                                   [d:\builds\seamonkey\mozilla\js\src\
jsinterp.c, line 687]
js_Interpret                                [d:\builds\seamonkey\mozilla\js\src\
jsinterp.c, line 2263]
js_Invoke                                   [d:\builds\seamonkey\mozilla\js\src\
jsinterp.c, line 687]
js_InternalInvoke                           [d:\builds\seamonkey\mozilla\js\src\
jsinterp.c, line 760]
JS_CallFunctionValue                        [d:\builds\seamonkey\mozilla\js\src\
jsapi.c, line 2773]
nsJSContext::CallEventHandler               [d:\builds\seamonkey\mozilla\dom\src\
base\nsJSEnvironment.cpp, line 568]
nsJSEventListener::HandleEvent              [d:\builds\seamonkey\mozilla\dom\src\
events\nsJSEventListener.cpp, line 129]
nsEventListenerManager::HandleEventSubTyp   [d:\builds\seamonkey\mozilla\layout\
events\src\nsEventListenerManager.cpp, line 643]
nsEventListenerManager::HandleEvent         [d:\builds\seamonkey\mozilla\layout\
events\src\nsEventListenerManager.cpp, line 1198]
GlobalWindowImpl::HandleDOMEvent            [d:\builds\seamonkey\mozilla\dom\src\
base\nsGlobalWindow.cpp, line 3251]
nsWebShell::OnEndDocumentLoad               [d:\builds\seamonkey\mozilla\
webshell\src\nsWebShell.cpp, line 3140]
nsDocLoaderImpl::FireOnEndDocumentLoad      [d:\builds\seamonkey\mozilla\
webshell\src\nsDocLoader.cpp, line 812]
nsDocLoaderImpl::DocLoaderIsEmpty           [d:\builds\seamonkey\mozilla\
webshell\src\nsDocLoader.cpp, line 714]
nsDocLoaderImpl::OnStopRequest              [d:\builds\seamonkey\mozilla\
webshell\src\nsDocLoader.cpp, line 658]
nsLoadGroup::RemoveChannel                  [d:\builds\seamonkey\mozilla\netwerk\
base\src\nsLoadGroup.cpp, line 545]
nsFileChannel::OnStopRequest                [d:\builds\seamonkey\mozilla\netwerk\
protocol\file\src\nsFileChannel.cpp, line 500]
nsOnStopRequestEvent::HandleEvent           [d:\builds\seamonkey\mozilla\netwerk\
base\src\nsAsyncStreamListener.cpp, line 279]
nsStreamListenerEvent::HandlePLEvent        [d:\builds\seamonkey\mozilla\netwerk\
base\src\nsAsyncStreamListener.cpp, line 94]
PL_HandleEvent                              [plevent.c, line 527]
_md_EventReceiverProc                       [plevent.c, line 977]
KERNEL32.DLL + 0x363b (0xbff7363b)          KERNEL32.DLL + 0x242e7 (0xbff942e7)         
0x00638c42 

Comment 3

[leger]

Adding "crash" keyword to all known open crasher bugs.

Comment 4

[Simon Fraser [no longer active]]

This is crashing in XP connect code, in the xptcall glue. No idea who should own 
this, but it's not me.

Here's a stack from Mac:

  Back chain  ISA  Caller
  00000000    PPC  1F4AFC34  
  0EF4A780    PPC  1F49A630  main+00240
  0EF4A6E0    PPC  1F498C14  main1(int, char**, nsISplashScreen*)+00810
  0EF4A5E0    PPC  1F338E38  nsAppShellService::Run()+000E0
  0EF4A590    PPC  1E82BF08  nsAppShell::Run()+00040
  0EF4A550    PPC  1E82C7B8  nsMacMessagePump::DoMessagePump()+00044
  0EF4A500    PPC  1E82CBBC  nsMacMessagePump::DispatchEvent(int, EventRecord*)+
001B0
  0EF4A4B0    PPC  1E8FDA24  Repeater::DoRepeaters(const EventRecord&)+0003C
  0EF4A460    PPC  1E806D60  nsMacNSPREventQueueHandler::RepeatAction(const 
EventRecord&)+00014
  0EF4A420    PPC  1E80703C  nsMacNSPREventQueueHandler::ProcessPLEventQueue()+
00274
  0EF4A390    PPC  1EA8FAFC  nsEventQueueImpl::ProcessPendingEvents()+00068
  0EF4A320    PPC  1EB06720  PL_ProcessPendingEvents+00084
  0EF4A2D0    PPC  1EB06864  PL_HandleEvent+00054
  0EF4A290    PPC  1E5DE540  nsStreamListenerEvent::HandlePLEvent(PLEvent*)+00050
  0EF4A250    PPC  1E5DFAF0  nsOnStopRequestEvent::HandleEvent()+00098
  0EF4A200    PPC  1E52A6C8  nsFileChannel::OnStopRequest(nsIChannel*, 
nsISupports*, unsigned int, const unsigned short*)+000D0
  0EF4A1A0    PPC  1E60A914  nsLoadGroup::RemoveChannel(nsIChannel*, nsISupports*
, unsigned int, const unsigned short*)+00440
  0EF4A100    PPC  1F225D2C  nsDocLoaderImpl::OnStopRequest(nsIChannel*, 
nsISupports*, unsigned int, const unsigned short*)+0009C
  0EF4A0A0    PPC  1F22613C  nsDocLoaderImpl::DocLoaderIsEmpty(unsigned int)+
00294
  0EF4A010    PPC  1F226768  
nsDocLoaderImpl::FireOnEndDocumentLoad(nsDocLoaderImpl*, nsIChannel*, unsigned 
int)+00328
  0EF49F80    PPC  1F454D9C  nsWebShell::OnEndDocumentLoad(nsIDocumentLoader*, 
nsIChannel*, unsigned int)+001B8
  0EF49890    PPC  1E924480  GlobalWindowImpl::HandleDOMEvent(nsIPresContext*, 
nsEvent*, nsIDOMEvent**, unsigned int, nsEventStatus*)+001FC
  0EF497F0    PPC  1E0C4E6C  nsEventListenerManager::HandleEvent(nsIPresContext*, 
nsEvent*, nsIDOMEvent**, unsigned int, nsEventStatus*)+01358
  0EF49650    PPC  1E0C37D0  
nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEvent*, 
unsigned int, unsigned int)+00860
  0EF49410    PPC  1E98D00C  nsJSEventListener::HandleEvent(nsIDOMEvent*)+001F0
  0EF492F0    PPC  1E91247C  nsJSContext::CallEventHandler(void*, void*, unsigned 
int, void*, int*)+004BC
  0EF49220    PPC  1EB37734  JS_CallFunctionValue+00044
  0EF491E0    PPC  1EB5620C  js_InternalInvoke+000D4
  0EF49120    PPC  1EB55F60  js_Invoke+006F4
  0EF49020    PPC  1EB5E990  js_Interpret+07CD0
  0EF48C70    PPC  1EA34CA4  WrappedNative_GetProperty(JSContext*, JSObject*, 
long, long*)+00244
  0EF48BC0    PPC  1EA329B4  
nsXPCWrappedNativeClass::CallWrappedMethod(JSContext*, nsXPCWrappedNative*, const 
XPCNativeMemberDescriptor*, nsXPCWrappedNativeClass::CallMode, unsigned int, 
long*, long*)+01174
  0EF488F0    PPC  1EAC9148  XPTC_InvokeByIndex+0002C

Comment 5

[John Bandhauer]

I'm afraid that this one is going to linger real long if it stays on my bug 
list. I find it very unlikely that the code is really failing where the stack 
stops. It seems far more likely to me that the debugger's stack walker is 
running into the xptcall boundary and not being able to traverse it to then show 
you the real crash location. Tell me I'm wrong.

OR

Maybe the referenced object is getting prematurely released due to incorrect 
reference counting somewhere?

Does anyone have any ideas on how to help reproduce and debug this?

Comment 6

[John Bandhauer]

So it turns out this crashes in the same way on Windows too.

The JS call stack looks like:

0 loadDialog() ["chrome://global/content/finddialog.js":35]
    this = [object Window]
1 onLoad() ["chrome://global/content/finddialog.js":102]
    this = [object Window]
2 onload(event = [object KeyEvent]) ["<unknown>":0]
    this = [object Window]
3 <TOP LEVEL> ["<unknown>":0]

[use http://www.mozilla.org/scriptable/javascript-stack-dumper.html]

in finddialog.js I find:

            // Turn "data" into weak reference.
            data.ConvertToWeakReference();

It crashes in the next access of "data" after this call...

dialog.findKey.setAttribute( "value", data.searchString );

Commenting out "data.ConvertToWeakReference();" makes it not crash.

See:
http://lxr.mozilla.org/seamonkey/search?string=ConvertToWeakReference

It sure looks to me like this rapid clicking on the "Find" button as the find 
dialog is being brought up is causing this ConvertToWeakReference() code to 
release the reference owned by xpconnect on the "data" object. The object 
goes away and xpconnect unknowingly calls into a bogus interface pointer. This 
is an uncooperative thing for this code to be doing.

It *might* be that this works in other cases by chance. Perhaps in other cases 
there is some other reference on the native object via JS and in this case the 
the garbage collection that is done as windows come and go is causing that 
(assumed) other reference by some nonrooted object to go away.

Like any other xpcom component, xpconnect can not be expected to successfully 
call methods on native objects if the Addref it has done to hold the object has 
been Released by some other means outside of its control.

I'm sure that there is some good reason why it was decided to structure this 
finddialog code in this way, but it is built on very shaky ground.

Comment 7

[John Bandhauer]

Perhaps nsIWeakReference is the answer if you need to be breaking a cycle here.

Comment 8

[don]

Bill, should this be fixed for M15?  I'm inclined to move it to M16.

Comment 9

[Bill Law]

Yes, there was a good reason why it the code was structured this way.  Like most 
code, it was done that way because it seemed to work (at the time :-).  There 
were a couple of complications relating to getting it so that closing the owning 
browser window closed the find dialog (but only if it was open).

Anyway, it worked at the time it was written (and didn't leak).  My concern with 
your fix is that you've removed the Release, but have left in the AddRef that's 
in the unload handler.  I think that this might result in the object getting 
leaked.

But I can't be sure without digging into it more.  This code might be affected 
by features/side-effects/bugs that are now present in the windowing system but 
weren't previously.

Perhaps it would be better to error on the side of safety (leak vs. crash) but 
I'm not anxious to tweak this code at this point, for M15.  I'd hope not too 
many people will go mashing repeatedly on the find button.  I can look at it for 
M16 (i.e., before beta2).

P.S. IIRC, I looked at using nsIWeakReference but it wouldn't apply for some 
reason (the exact nature of which escapes me now).  Maybe it was because I 
wanted the weak reference to be in the xpconnect object that held the reference 
to the real object?

Comment 10

[John Bandhauer]

FWIW, I didn't mean to suggest that commenting out data.ConvertToWeakReference() 
would be the right fix - just showing an experiment that made me think that 
the Release done there was the source of the crash.

It is a pain, but it seems to me that you could write a fairly simple JS object 
that would hold a reference on the native nsIWeakReference object and use  
QueryReferent internally to get data and make calls on the underlying native as 
needed. Or, it could just expose QueryReferent in some way. Either way, 
xpconnect would hold a longer term reference to the nsIWeakReference object and 
your code could minimize the holding of the underlying object by zeroing out any 
references to it immediately after each use.

Comment 11

[don]

Move to M20 target milestone.

Comment 12

[johng]

nav triage team:
Doesn't seem to be a problem anymore.  Works for me.

Comment 13

[sujay]

verified in 8/18 build.