Closed Bug 139556 Opened 22 years ago Closed 22 years ago

Crash in js_FreeStack [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 409]

Categories

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

Product:
Core
Component:
DOM: Events
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: attinasi, Assigned: dougt)

References

Details

(Keywords: crash, topembed+, Whiteboard: [hitlist] [adt2 rtm] [ETA 06/20])

Attachments

(4 files, 7 obsolete files)

Reduced HTML testcase
317 bytes, text/html
Details
WinNT stack trace
3.91 KB, text/plain
Details
Simpified necko changes
1.60 KB, patch
rpotts
: review+
darin.moz
: superreview+
asa
: approval+
Details | Diff | Splinter Review
patch 5
18.49 KB, patch
rpotts
: review+
darin.moz
: superreview+
jesup
: approval+
Details | Diff | Splinter Review 
This bug is a spinoff from bug 118014. Crash is in javascript engine somewhere now.

See talkback report:
http://climate.mcom.com/reports/singleincidentinfo.cfm?dynamicBBID=TB5484538k
From bug 118014, the steps to reporoduce are:

Re-tested this on netscape 4-18 100 build using the following steps: 

1. navigate to www.ibm.com
2. click ebusiness link (red 'e'on right hand side of page) 
3. click glossary 
4. begin navigating back and forth quickly between pages 
Result: Crash now occurs in js3250.dll. Talkback id is: TB5484538k
Summary: Crash in js_FreeStack [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 409] → Crash in js_FreeStack [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 409]
Keywords: topembed+
Unable to reproduce this in mozilla or TestGtkEmbed with a current branch build
on Linux.  Investigating on win32.  Here is the stack, copied from bug 118014:

js_FreeStack [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 409]
JS_PopArguments [d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 391]
nsJSEventListener::HandleEvent
[d:\builds\seamonkey\mozilla\dom\src\events\nsJSEventListener.cpp, line 186]
nsEventListenerManager::HandleEventSubType
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, line
1218]
nsEventListenerManager::HandleEvent
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, line
1893]
GlobalWindowImpl::HandleDOMEvent
[d:\builds\seamonkey\mozilla\dom\src\base\nsGlobalWindow.cpp, line 741]
nsDocument::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsDocument.cpp, line 3241]
nsGenericElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericElement.cpp, line 1673]
nsGenericElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericElement.cpp, line 1673]
nsGenericElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericElement.cpp, line 1673]
nsHTMLScriptElement::ScriptAvailable
[d:\builds\seamonkey\mozilla\content\html\content\src\nsHTMLScriptElement.cpp,
line 339]
nsScriptLoadRequest::FireScriptAvailable
[d:\builds\seamonkey\mozilla\content\base\src\nsScriptLoader.cpp, line 113]
nsScriptLoader::FireScriptAvailable
[d:\builds\seamonkey\mozilla\content\base\src\nsScriptLoader.cpp, line 505]
nsScriptLoader::OnStreamComplete
[d:\builds\seamonkey\mozilla\content\base\src\nsScriptLoader.cpp, line 617]
nsStreamLoader::OnStopRequest
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsStreamLoader.cpp, line 163]
nsHttpChannel::OnStopRequest
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpChannel.cpp, line 2829]
nsOnStopRequestEvent::HandleEvent
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsRequestObserverProxy.cpp, line 213]
PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 597]
PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c,
line 530]
_md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line
1078]
nsAppShellService::Run
[d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsAppShellService.cpp, line 309]
main1 [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1431]
main [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1766]
WinMain [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1784]
WinMainCRTStartup()
KERNEL32.DLL + 0x7903 (0x77e87903) 

Jen... Can you help repro?
I cannot reproduce this on win2k with a current moz100 branch build with either
mozilla or mfcembed
I am able to reproduce this with current Mozilla trunk builds on WinNT.
I'll attach a reduced testcase below, and a stack trace from a debug build -
Attached file Reduced HTML testcase — 

    
    

    
  
Here is the source of the reduced testcase:

<html>
<head><title>IBM e-business Homepage</title>

<script type="text/javascript" language="JavaScript1.2" 
src="http://www.ibm.com/common/stats/stats.js"></script>

<script type="text/javascript" language="JavaScript1.2" 
src="http://www.ibm.com/e-business/global/js/tracking.js"></script>

</head></html>



STEPS TO REPRODUCE
1. Use a DEBUG Mozilla build, not binaries (which seem too fast for this)
2. Load the reduced HTML testcase
3. Load about:blank
4. Click on the Back button, then the Forward button, as fast as you can
5. Repeat step 4 until you crash (could be over 20 or 30 times)


Will attach stack trace below.
Using trunk Mozilla debug build 2002-04-22.
OS: WinNT 4.0 (SP6)
Assignee: rogerl → khanson

    
    

    
  

      
      
      
      

        Attached file
          
        WinNT stack trace
        — 
      

    


  

    
    

    
  
dude, if I have to repeat this 20 to 30 times to maybe reproduce it, why in the
world is this a beta 3 blocker?

    
    

    
  
that said, I just tried and other than moving my right hand a little closer to a
nice case of tendanitis, I couldn't reprodoce on my mozilla 100 branch build on
win2k on a fast machine

    
    

    
  
The real-life site crashes more easily than the testcase.
In fact, I can only crash the testcase with a debug build.

It takes more effort to crash the testcase because it is
so much reduced from the real-life site. Nevertheless, the
stack trace it generates is exactly the same. The testcase
will be of more use to anyone who has to look into this.

It comes from the e-business link in step 2 of Comment #1,
by stripping more and more out of it. The more I left in,
the easier it was to crash by toggling from this to about:blank
and back. (about:blank chosen for simplicity). 

People who set priority might try the steps in Comment #1
to decide how important this bug really is.

cc'ing jband: this is the js_FreeStack() crash I mentioned -

    
  
Keywords: crash

    
    

    
  
ADT, Winnie:  This bug came off bug 118014 which was deemed a critical bug. 
Please review this bug and prioritize.  Meanwhile it's on my hitlist until you
say differently.
Whiteboard: [hitlist]

    
    

    
  
nsbeta1+/adt2 (severe because it is a crash, but we don't yet know if it is a
topcrasher) as this bug is a spinoff from bug 118014, which was a top adt1 bug.
Keywords: nsbeta1+
Whiteboard: [hitlist] → [hitlist] [adt2]

    
    

    
  
i believe I have a fix for this.  

    
    

    
  
over to me since I am working on a fix.
Assignee: khanson → dougt

    
    

    
  
Here is a summary of the problem and a fix.  

We have an class nsScriptLoadContext which is passed as a context to a necko
load request.  The lifetime of this context object is such that the only
remaining reference is held onto by necko.  When the necko load is complete,
necko releases the object without any thoughts as to the objects threadsafety. 
The object implements nsISupport with a threadsafe version, but that is not
enough.  This release causes the destruction of the object on a unspecified
thread.  This race condition is harmful especially when stuff going on; results
are completely unpredictable.  

I guess the best thing to do is have a generic way that necko can ensure where
the content releases happens.  This would allow anyone to pass in any context
nsISupport and not worry about its threadsafety.  However, on flip side, there
are many context objects which are clearly at home being truely threadsafe. 
Somewhere in the middle is the vast majority of the context objects who's
lifetime outlives the necko request and implement nsISupports in a threadsafe
way.  This majority has really no need for ensuring where the release happens.  

Looking at fixing necko is a large problem.  We have to ensure that all contexts
are properly owned.  Right now, transports and event proxies both own the
context.  We need to ensure that when we are ready to post the onStopRequest
event, that only one reference is held to this context by necko.  This is going
to be a mini nightmare to clean up in the short term.  

Given the risks involved, I suggest that we just ensure that the necko contexts
which have lifetime issues proxy their final delete to the ui thread.  This
would ensure no increased risks to the many other necko contexts.  The change
should primarly be in xpcom/proxy.  I can expose code which has been used for
months now which can allow the nsScriptLoadContext to ensure its delete happens
in the right place.

The fix is to make a macro name NS_IMPL_PROXY_RELEASE.  The contents of this
macro is mostly extracted from existing code in my proxy code.  When a class
defines its release method with this macro, the delete will be "posted" via a
sync even to the ui event queue.  I was thinking about making an async version.
 This can be done later if we want to see if it makes any performance difference.

I have tested this with the simplifed test case and am no longer able to crash.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        xpcom/proxy patch. (obsolete)
        — 
          — Splinter Review
      

    


  
Patch exposes NS_IMPL_PROXY_RELEASE

    
    

    
  


  

    
    

    
  
both patches are required.

    
    

    
  


  
Properly nulls contexts

        Attachment #81013 -
        Attachment is obsolete: true

        Attachment #81014 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 81033 [details] [diff] [review]
Simpified necko changes

r=rpotts@netscape.com

        Attachment #81033 -
        Flags: review+

    
    

    
  
Comment on attachment 81033 [details] [diff] [review]
Simpified necko changes

via aim r=rpotts@netscape.com

    
    

    
  
Comment on attachment 81033 [details] [diff] [review]
Simpified necko changes

sr=darin

looks good... of course, there's still a race condition, that i suspect we'll
win like 99.99% of the time, but eventually we should try to solve this for
real.

can you file a bug to close out the race condition?

        Attachment #81033 -
        Flags: superreview+

    
  
Keywords: adt1.0.0, 
          
            edt1.0.0

    
    

    
  
Checked onto trunk

Checking in nsFileTransport.cpp;
/cvsroot/mozilla/netwerk/base/src/nsFileTransport.cpp,v  <--  nsFileTransport.cpp
new revision: 1.126; previous revision: 1.125
done
Checking in nsSocketTransport.cpp;
/cvsroot/mozilla/netwerk/base/src/nsSocketTransport.cpp,v  <-- 
nsSocketTransport.cpp
new revision: 1.238; previous revision: 1.237
done


    
    

    
  
Comment on attachment 81033 [details] [diff] [review]
Simpified necko changes

a=asa (on behalf of drivers) for checkin to the 1.0 branch

        Attachment #81033 -
        Flags: approval+

    
    

    
  
*** Bug 139371 has been marked as a duplicate of this bug. ***

    
    

    
  
Since the fix involves Networking code and not JS Engine,
changing component accordingly -
Component: JavaScript Engine → Networking
QA Contact: pschwartau → benc

    
    

    
  
adt1.0.0+ (on ADT's behalf) for checkin to the 1.0 branch. Pls check this one is
asap, then add the keyword fixed1.0.0. thanks!
Keywords: adt1.0.0 → adt1.0.0+
Priority: -- → P1
Target Milestone: --- → mozilla1.0

    
    

    
  
Checking in nsFileTransport.cpp;
/cvsroot/mozilla/netwerk/base/src/nsFileTransport.cpp,v  <--  nsFileTransport.cpp
new revision: 1.125.2.2; previous revision: 1.125.2.1
done
Checking in nsSocketTransport.cpp;
/cvsroot/mozilla/netwerk/base/src/nsSocketTransport.cpp,v  <-- 
nsSocketTransport.cpp
new revision: 1.237.2.2; previous revision: 1.237.2.1
done

Status: NEW → RESOLVED
Closed: 22 years ago
Keywords: fixed1.0.0
Resolution: --- → FIXED

    
    

    
  
Testing with a branch pull from 4/26 failure still occured:
TalkBack Incident ID#: TB5735353M

Its possible that the "fix" landed a bit later than the Mozilla pull. But we
need to check if possible the issue is in fact resolved.

    
    

    
  
I checked in this change at 2002/04/25 21:04:26;  reopening.

still occuring... 
Status: RESOLVED → REOPENED
Component: Networking → Layout
Resolution: FIXED → ---

    
    

    
  
dougt: did you want this to go to layout?

    
    

    
  
removing the fixed1.0.0 keyword until we're in the clear.
Keywords: fixed1.0.0

    
    

    
  
I can not reproduce this.  Over to default owner...
Assignee: dougt → attinasi
Status: REOPENED → NEW
QA Contact: benc → petersen

    
    

    
  
What is the layout work that needs to be done for this bug? I don't see layout
in the stack trace anywhere.

    
    

    
  
didn't want this to go to layout.  Dom Events for investigation.  I thought that
this problem was caused by a race, but apparently not.
Assignee: attinasi → joki
Component: Layout → DOM Events
QA Contact: petersen → vladimire

    
    

    
  
There was a patch checked in here, but it didn't fix the problem ... should the
patch still be kept on the 1.0 branch (i.e. We might have introduced risk,
without any real benefit)? Removing adt1.0.0+, until there is a patch that
addresses this crash with reviews.
Keywords: adt1.0.0+

    
    

    
  
Jaime - the patch fixes the crash for me.


Michael - how often are you seeing this crash post my fix?

    
    

    
  
Looks like the treebody frame is being destroyed somewhere w/o the
nsTreeBoxObject being notified about the destruction of the treebody frame. This
is similar to what happened in bug 138138, but I don't know off hand what
triggers this situation here. In 138138 the presentation was torn down due to
the document viewer being hidden but the xul document wasn't notified about this
so its boxobject hash stayed around holding on to presentation data (i.e.
nsIFrame's) past the destruction of the presentation data. This seems related,
but also different...

    
    

    
  
Um, wrong bug, ignore my last comment :-)

    
    

    
  
Doug - Can you explain comment #33 From Judson Valeski, and why the bug is still
open, if your patch fixes the crash?

    
    

    
  
Jud can explain his own comments. 

Why this was reopened is that we still have talkback data that suggest that this
crash is occuring.  Like I said, I couldn't reproduce it in my build, but I am
running a debug version which probably pads the timing somewhat.  

>(i.e. We might have introduced risk, without any real benefit)

The patch makes things better.  We reduced the chance of losing the race. We may
need to fix this "for real" since it appears that we can still trigger the race.
 This will require more work, but I want to make sire that their isn't something
that DOM Events is doing that could cause this crash.  Vidur, Jst?







    
    

    
  
I haven't been able to reproduce this either (Win2k trunk build).  As far as DOM
events specific stuff, we use the same JS-XPCOM layer as the rest of the DOM. 
We don't do anything specific to handle JS level code, though the event system
may expose a problem in the scripting layer that other code doesn't due to
reentrancy, threading, etc.  In any case I don't have any particular insights on
what might cause this in event code.  I would defer to jst or jband at this level.

    
    

    
  
Jan, any thoughts on the tree scenario described in comment 39?

    
    

    
  
Oops, wrong bug (how about if I read _all_ the comments)

    
    

    
  
I talked it over with jst and we both agree that events aren't doing anything 
odd that would make them a likely culprit.  It seems likely that doug is right 
and the crash is race is still ocurring, even if the freqency was lowered.  If 
expertise is needed at the XPConnect level then jst or jband is the right person 
to provide it but until then doug still seems like the best owner for this.
Assignee: joki → dougt

    
  
Blocks: 143047

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Really fixes the race (obsolete)
        — 
          — Splinter Review
      

    


  
As Darin and I mentioned, patch 81033 only reduced the chance of losing a race
condition.  We didn't believe the loss would occur as frequently as it does.  

Attached is a patch which addresses the underlying context ownership problems
with the socket and file transport.

    
    

    
  
Comment on attachment 84116 [details] [diff] [review]
Really fixes the race

spoke with dougt about this patch via AIM... we came up with some things that
need to change.

        Attachment #84116 -
        Flags: needs-work+

    
    

    
  
FYI, I saw this problem earlier today when loading http://www.netscape.com

    
    

    
  
jst: we have a fix in mind.  just need to clean up some stuff.  I will see if I
can get a patch over the weekend, latest monday night.  



    
    

    
  
Awesome!

    
    

    
  
ugh.
the fix has some performance problems that need to be worked out.

    
    

    
  
so, having the nsRequestObserverProxy, nsStreamListenerProxy, or
nsStreamProviderProxy do some magic to release the objects on the right thread
does not completely solve the problem as the last remaining reference is with
the transport not the proxy.  Yes, we could work out some semantic which would
allow us to workaround this problem.  In fact, this is what my last patch (which
i didn't post) did.  We would init the proxy with the context and have the
context be owened by the proxy.  This kinda works, but the context is also
required by the transports to do progress notification.  To hell with that approach.

The contract on these interfaces is that context and the
listener/provider/observer is to be released on the thread which started the
load (asyncRead, asyncWrite).  It turns out that a cleaner solution is having
the transports just post release events for the objects that must be released on
a certain thread.  

patch coming up...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v.1 (obsolete)
        — 
          — Splinter Review
      

    


  
Never describe a bug as "really fixes", or "Final".  -- Murphy's Law of
Software Patch Evolution.

darin, see my comment in netwerk/base/src/nsRequestObserverProxy.cpp.	I do
not think that with my patch we will need this anymore.

        Attachment #84116 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v.2 (obsolete)
        — 
          — Splinter Review
      

    


  
fixed mistake in END_WRITE - deleting the wrong thing..  thx darin

        Attachment #84490 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 84490 [details] [diff] [review]
patch v.1

>Index: base/src/nsFileTransport.cpp

>+        if (mProvider) {
>+            mProvider->OnStopRequest(this, mContext, mStatus);
>+        }
>+        if (mEventQ) {
>+            if (mListener) {

shouldn't this be mProvider?

>+            mListener = nsnull;

here too.


>Index: base/src/nsRequestObserverProxy.cpp
>+        ProxyRelease(mEventQ, obs);  // I do not think this is needed since we 
>+                                     // are moving this logic into the transport.

this is still needed as nsRequestObserverProxy should cleanup all of its
member vars on the right thread.  HTTP, for example, uses a
nsRequestObserverProxy independent of the transports.


>Index: base/src/nsSocketTransport.cpp

>+    if (mEventQ) {
>+        if (mListener) {
>+            nsISupports* doomed = mListener.get();
>+            NS_ADDREF(doomed);
>+            mListener = 0;
>+            NS_ProxyRelease(mEventQ, doomed);
>+        }
>+    }
>+    else {
>+        mListener = nsnull;
>+    }
>     return nsSocketRequest::OnStop();

ugh.. often mListener is the same as nsSocketRequest::mObserver.  too bad
we can't coalesce these releases.  likewise, for mProvider.

otherwise, this looks good... i wish the nsIStreamListener, etc. proxy code
handle all of this behind the scenes (since it is used elsewhere), but i 
understand how difficult that is.  hopefully, these extra events won't
show up on the perf radar... they shouldn't since we're only adding an
extra event or two per transport.

with these few things fixed and lot's of testing... r/sr=darin

        Attachment #84490 -
        Attachment is obsolete: false

        Attachment #84490 -
        Flags: needs-work+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v.3 (obsolete)
        — 
          — Splinter Review
      

    


  
darin and I spoke about this.  the listener and provider do not need to be
proxy released.

        Attachment #84490 -
        Attachment is obsolete: true

        Attachment #84496 -
        Attachment is obsolete: true

    
    

    
  
This is wacky :-)

>+        if (mListener) {
>+            nsISupports* doomed = mListener.get();
>+            NS_ADDREF(doomed);
>+            mListener = 0;
>+            NS_ProxyRelease(mEventQ, doomed);
>+        }

Shouldn't the ADDREF be done inside of NS_ProxyRelease(...) when the ISupports*
is put into the PLEvent struct?

-- rick
 

    
    

    
  
this would create the same race condition as we are trying to solve.  I am
transfering ownership from the comptr to a raw pointer so that i can assume that
the call to NS_ProxyRelease will release the only reference to the object.

Assuming I don't do it this way:

+            nsISupports* doomed = mListener.get();
+            NS_ProxyRelease(mEventQ, doomed);

<thread context switch>
< proxy release event is handled.  You didn't think we could handle events this
quick! >

<thread context switch>

+            mListener = 0;  <-- object is still references on original thread!


    
    

    
  
right, the AddRef could be avoided if there was a way to null out a COMptr
without the COMptr calling Release on the underlying nsISupports pointer.

    
    

    
  
Comment on attachment 85313 [details] [diff] [review]
patch v.3

>Index: netwerk/base/src/nsFileTransport.cpp

>+            if (mEventQ) {
>+                nsISupports* doomed = mContext.get();
>+                NS_ADDREF(doomed);
>+                mContext = 0;
>+                NS_ProxyRelease(mEventQ, mContext);

whoops!  shouldn't this be

		  NS_ProxyRelease(mEventQ, doomed);

??

likewise in the mProvider case.  nsSocketTransport appears to have the same
bug.

otherwise, the patch looks good.  sr=darin with this fix.

        Attachment #85313 -
        Flags: needs-work+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        PATCH V.4 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #85313 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 85509 [details] [diff] [review]
PATCH V.4

from darin

        Attachment #85509 -
        Flags: superreview+

    
    

    
  
Summary:

The basic problem is that the file and socket transport threads owns the last
reference to the user context.  Necko is suppose to ensure that the context is
released on the same thread that started the request.  The current code has a
race condition in which a thread context switch occurs after we post an
OnStopRequest notification and before the release of the context:

context.mRefCnt is 2;

 listener->OnStopRequest(this, context, mStatus);

<thread switch to thread 1>
<context is released - mRefCnt is 1>
<thread switch back to thread 2>

 context = 0;  <-- caused delete context on wrong thread!

As you can see, this is bad.  The fix is to have the transport threads proxy the
final release.  We were already doing this in a few places in necko
(nsRequestObserveProxy).  



    
    

    
  
hey doug,

i think you're misunderstanding my question... i understand that the final
release must be done on the UI thread...  my question is why you are doing the
AddRef() *before* calling the function, rather than inside of the function right
after the call to PL_InitEvent() ??

This is what i'm sugesting:

+        // if we have a context, we have to ensure that it is released on the
+        // proper thread.
+        if (mContext) {
+            if (mEventQ) {
+                nsComPtr<nsISupports> doomed(mContext);
+                mContext = 0;
+                NS_ProxyRelease(mEventQ, doomed);
+            }
+            else {
+                mContext = nsnull;
+            }
+        }
 

+static void
+NS_ProxyRelease(nsIEventQueue *eventQ, nsISupports *doomed, PRBool
alwaysProxy=PR_FALSE)
+{
+   if (!doomed)
+      return;
+
+   if (!eventQ) {
---    NS_RELEASE(doomed);  // Don't release because it hasn't been addrefed
+      return;
+   }
+
+   if (!alwaysProxy) {
+      PRBool onCurrentThread = PR_FALSE;
+      eventQ->IsQueueOnCurrentThread(&onCurrentThread);
+      if (onCurrentThread) {
---      NS_RELEASE(doomed);
+         return;
+      }
+   }
+
+   PLEvent *ev = new PLEvent;
+   if (!ev) {
+      NS_ERROR("failed to allocate PLEvent");
+      return; // Now, we won't leak the object !!
+   }
+
+   PL_InitEvent(ev, 
+                (void *) doomed,
+                ReleaseDestructorEventHandler,
+                ReleaseDestructorDestroyHandler);
+   
+++ NS_ADDREF(doomed);
+
+   PRStatus rv = eventQ->PostEvent(ev);
+   NS_ASSERTION(rv == PR_SUCCESS, "PostEvent failed");
+}



In this situation, the owning reference is made inside of the NS_ProxyRelease()
function *only* when necessary...  It makes the ownership model much clearer. 
Because the owning reference is attached to the pointer held by the PLEvent...

-- rick


    
    

    
  
+            if (mEventQ) {
+                nsComPtr<nsISupports> doomed(mContext);
+                mContext = 0;

* doomed is the only reference here.

+                NS_ProxyRelease(mEventQ, doomed);

NS_ProxyRelease addrefs, posts the event, and returns.  

* doomed has a two references here.

<thread switch to UI>
<release event is processed>
* doomed has one reference again here.
<thread switch>

last reference to doomed is released <------
+            }
+            else {

make sense?

    
    

    
  
Comment on attachment 85509 [details] [diff] [review]
PATCH V.4


>+   PLEvent *ev = new PLEvent;
>+   if (!ev) {
>+      NS_ERROR("failed to allocate PLEvent");
>+      return;
>+   }

looks like you need a NS_RELEASE(doomed) here as well.

dougt: i caught up with rick and talked about this one a bit... he's with us on
this one... though
he suggested that we add a comment to indicate that there is some really wicket
reference counting
foo going on here.  can you add a comment to that effect?  thx!

        Attachment #85509 -
        Flags: needs-work+

    
  
Whiteboard: [hitlist] [adt2] → [hitlist] [adt2 rtm]

    
    

    
  
crap, if I can't new a PLEvent, we are in already in a world of pain.  i
disagree with releasing as you state as i would rather leak an object than
release an object on the wrong thread and crash.  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch 5 
        — 
          — Splinter Review
      

    


  
only comment changes.

        Attachment #85509 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 85707 [details] [diff] [review]
patch 5 

good point.. sr=darin

        Attachment #85707 -
        Flags: superreview+

    
  
Keywords: mozilla1.0.1

    
    

    
  
Fixed checked into trunk.
Status: NEW → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED

    
  
Keywords: adt1.0.1

    
    

    
  
vladimire - can you verify this on the trunk, and check around for any possible
regressions? thanks!

    
    

    
  
still crashes with phil's testcase, also crashes when I go to ibm.com
I am using build 2002-06-07-04-trunk on windows 98
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Removing adt1.0.1, as this has been reopened, and QA is still able to reproduce
the crash.
Keywords: adt1.0.1

    
    

    
  
may be a different bug since Vladimir is crashing just going to ibm.com.  We
need a stack trace as soon as possible.  I have also asked Vladimir to try this
in the Mozilla 1.0 build.

    
    

    
  
I opened up bug 149986 for the crash problem on ibm.com that I am experiencing.

    
    

    
  
Vladimir/DougT - Can this be marked as VErified/Fixed, then for the trunk? Is
yes, then pls mark it as Resolved/Fixed, then add adt1.0.1 to nominate it for
the 1.0 branch.

    
  
Blocks: 150716

    
    

    
  
This bug broke Compact folders on trunk bug 150716. If I back out the fix for
this bug it fixes the problem for me. looks like there could be some timing issue. 

    
    

    
  
I do not think that this crash is related to the crash the report is seeing.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED

    
    

    
  
verifying on build 2002-06-12-08 on Win 98 and Linux Red Hat.

Status: RESOLVED → VERIFIED

    
  
Keywords: adt1.0.1

    
    

    
  
adt1.0.1+ (on ADT's behalf) for checkin to the 1.0 branch, pending drivers'
approval. pls check this in asap, then add the "fixed1.0.1" keyword.
Keywords: adt1.0.1 → adt1.0.1+

    
    

    
  
Comment on attachment 85707 [details] [diff] [review]
patch 5 

Approved for 1.0 branch with a proviso: there are changes to observe XPCOM
shutdown in the DNS service in the patch.  Please verify that they're a needed
part of this patch before applying them (and comment here).  If they're
accidental, please do not check them in.

When checked in, please remove mozilla1.0.1+ and add fixed1.0.1

        Attachment #85707 -
        Flags: approval+

    
  
Whiteboard: [hitlist] [adt2 rtm] → [hitlist] [adt2 rtm] [ETA 06/20]

    
  
Keywords: mozilla1.0.1+ → fixed1.0.1

    
    

    
  
Verifying on 07/17 branch builds on windows 98 and linux RedHat
Keywords: fixed1.0.1 → verified1.0.1

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: