Closed
Bug 139556
Opened 23 years ago
Closed 23 years ago
Crash in js_FreeStack [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 409]
Categories
(Core :: DOM: Events, defect, P1)
Core
DOM: Events
Tracking
()
VERIFIED
FIXED
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)
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
rpotts
:
review+
darin.moz
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
(deleted),
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
Reporter | ||
Comment 1•23 years ago
|
||
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]
Comment 2•23 years ago
|
||
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)
Comment 3•23 years ago
|
||
Jen... Can you help repro?
Comment 4•23 years ago
|
||
I cannot reproduce this on win2k with a current moz100 branch build with either
mozilla or mfcembed
Comment 5•23 years ago
|
||
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 -
Comment 6•23 years ago
|
||
Comment 7•23 years ago
|
||
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
Comment 8•23 years ago
|
||
Comment 9•23 years ago
|
||
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?
Comment 10•23 years ago
|
||
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
Comment 11•23 years ago
|
||
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 -
Comment 12•23 years ago
|
||
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]
Comment 13•23 years ago
|
||
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]
Assignee | ||
Comment 14•23 years ago
|
||
i believe I have a fix for this.
Assignee | ||
Comment 16•23 years ago
|
||
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.
Assignee | ||
Comment 17•23 years ago
|
||
Patch exposes NS_IMPL_PROXY_RELEASE
Assignee | ||
Comment 18•23 years ago
|
||
Assignee | ||
Comment 19•23 years ago
|
||
both patches are required.
Assignee | ||
Comment 20•23 years ago
|
||
Properly nulls contexts
Attachment #81013 -
Attachment is obsolete: true
Attachment #81014 -
Attachment is obsolete: true
Comment 21•23 years ago
|
||
Attachment #81033 -
Flags: review+
Assignee | ||
Comment 22•23 years ago
|
||
Comment on attachment 81033 [details] [diff] [review]
Simpified necko changes
via aim r=rpotts@netscape.com
Comment 23•23 years ago
|
||
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+
Assignee | ||
Updated•23 years ago
|
Assignee | ||
Comment 24•23 years ago
|
||
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 25•23 years ago
|
||
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+
Assignee | ||
Comment 26•23 years ago
|
||
*** Bug 139371 has been marked as a duplicate of this bug. ***
Comment 27•23 years ago
|
||
Since the fix involves Networking code and not JS Engine,
changing component accordingly -
Component: JavaScript Engine → Networking
QA Contact: pschwartau → benc
Comment 28•23 years ago
|
||
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!
Assignee | ||
Comment 29•23 years ago
|
||
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
Comment 30•23 years ago
|
||
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.
Assignee | ||
Comment 31•23 years ago
|
||
I checked in this change at 2002/04/25 21:04:26; reopening.
still occuring...
Status: RESOLVED → REOPENED
Component: Networking → Layout
Resolution: FIXED → ---
Comment 32•23 years ago
|
||
dougt: did you want this to go to layout?
Comment 33•23 years ago
|
||
removing the fixed1.0.0 keyword until we're in the clear.
Keywords: fixed1.0.0
Assignee | ||
Comment 34•23 years ago
|
||
I can not reproduce this. Over to default owner...
Assignee: dougt → attinasi
Status: REOPENED → NEW
QA Contact: benc → petersen
Comment 35•23 years ago
|
||
What is the layout work that needs to be done for this bug? I don't see layout
in the stack trace anywhere.
Assignee | ||
Comment 36•23 years ago
|
||
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
Comment 37•23 years ago
|
||
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+
Assignee | ||
Comment 38•23 years ago
|
||
Jaime - the patch fixes the crash for me.
Michael - how often are you seeing this crash post my fix?
Comment 39•23 years ago
|
||
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...
Comment 40•23 years ago
|
||
Um, wrong bug, ignore my last comment :-)
Comment 41•23 years ago
|
||
Doug - Can you explain comment #33 From Judson Valeski, and why the bug is still
open, if your patch fixes the crash?
Assignee | ||
Comment 42•23 years ago
|
||
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?
Comment 43•23 years ago
|
||
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.
Comment 44•23 years ago
|
||
Jan, any thoughts on the tree scenario described in comment 39?
Comment 45•23 years ago
|
||
Oops, wrong bug (how about if I read _all_ the comments)
Comment 46•23 years ago
|
||
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
Assignee | ||
Comment 47•23 years ago
|
||
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 48•23 years ago
|
||
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+
Comment 49•23 years ago
|
||
FYI, I saw this problem earlier today when loading http://www.netscape.com
Assignee | ||
Comment 50•23 years ago
|
||
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.
Comment 51•23 years ago
|
||
Awesome!
Assignee | ||
Comment 52•23 years ago
|
||
ugh.
the fix has some performance problems that need to be worked out.
Assignee | ||
Comment 53•23 years ago
|
||
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...
Assignee | ||
Comment 54•23 years ago
|
||
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
Assignee | ||
Comment 55•23 years ago
|
||
fixed mistake in END_WRITE - deleting the wrong thing.. thx darin
Attachment #84490 -
Attachment is obsolete: true
Comment 56•23 years ago
|
||
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+
Assignee | ||
Comment 57•23 years ago
|
||
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
Comment 58•23 years ago
|
||
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
Assignee | ||
Comment 59•23 years ago
|
||
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!
Comment 60•23 years ago
|
||
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 61•23 years ago
|
||
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+
Assignee | ||
Comment 62•23 years ago
|
||
Attachment #85313 -
Attachment is obsolete: true
Assignee | ||
Comment 63•23 years ago
|
||
Comment on attachment 85509 [details] [diff] [review]
PATCH V.4
from darin
Attachment #85509 -
Flags: superreview+
Assignee | ||
Comment 64•23 years ago
|
||
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).
Comment 65•23 years ago
|
||
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
Assignee | ||
Comment 66•23 years ago
|
||
+ 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 67•23 years ago
|
||
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+
Updated•23 years ago
|
Whiteboard: [hitlist] [adt2] → [hitlist] [adt2 rtm]
Assignee | ||
Comment 68•23 years ago
|
||
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.
Assignee | ||
Comment 69•23 years ago
|
||
only comment changes.
Attachment #85509 -
Attachment is obsolete: true
Comment 70•23 years ago
|
||
Comment on attachment 85707 [details] [diff] [review]
patch 5
good point.. sr=darin
Attachment #85707 -
Flags: superreview+
Comment 71•23 years ago
|
||
Attachment #85707 -
Flags: review+
Assignee | ||
Updated•23 years ago
|
Keywords: mozilla1.0.1
Assignee | ||
Comment 72•23 years ago
|
||
Fixed checked into trunk.
Status: NEW → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 73•23 years ago
|
||
vladimire - can you verify this on the trunk, and check around for any possible
regressions? thanks!
Comment 74•23 years ago
|
||
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 → ---
Comment 75•23 years ago
|
||
Removing adt1.0.1, as this has been reopened, and QA is still able to reproduce
the crash.
Keywords: adt1.0.1
Assignee | ||
Comment 76•23 years ago
|
||
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.
Comment 77•23 years ago
|
||
I opened up bug 149986 for the crash problem on ibm.com that I am experiencing.
Comment 78•23 years ago
|
||
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.
Comment 79•23 years ago
|
||
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.
Assignee | ||
Comment 80•23 years ago
|
||
I do not think that this crash is related to the crash the report is seeing.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 81•23 years ago
|
||
verifying on build 2002-06-12-08 on Win 98 and Linux Red Hat.
Status: RESOLVED → VERIFIED
Comment 82•23 years ago
|
||
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.
Comment 83•23 years ago
|
||
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+
Updated•23 years ago
|
Keywords: mozilla1.0.1 → mozilla1.0.1+
Updated•23 years ago
|
Whiteboard: [hitlist] [adt2 rtm] → [hitlist] [adt2 rtm] [ETA 06/20]
Assignee | ||
Updated•23 years ago
|
Keywords: mozilla1.0.1+ → fixed1.0.1
Comment 84•23 years ago
|
||
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.
Description
•