Last Comment Bug 496774 - XPCJSRuntime::XPCJSRuntime shouldn't touch mJSRuntime without null checking something
: XPCJSRuntime::XPCJSRuntime shouldn't touch mJSRuntime without null checking s...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.9.2b1
Assigned To: timeless
:
Mentors:
http://mxr-test.konigsberg.mozilla.or...
: 496628 (view as bug list)
Depends on:
Blocks: 477187
  Show dependency treegraph
 
Reported: 2009-06-07 00:57 PDT by timeless
Modified: 2010-01-29 07:15 PST (History)
11 users (show)
sayrer: blocking1.9.2+
sayrer: wanted1.9.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed
.8-fixed


Attachments
only create mWatchdogThread if mWatchdogWakeup is around to release it (1022 bytes, patch)
2009-06-07 01:06 PDT, timeless
bent.mozilla: review+
jst: superreview+
dveditz: approval1.9.1.8+
Details | Diff | Review

Description timeless 2009-06-07 00:57:22 PDT
hi gal. please be more careful about using locks which are otherwise null checked.

I'm not sure what this code is doing, but you're the last person to add code which doesn't make sense. And we have reports of crashes (bug 496628, mostly incomprehensible) involving a PR_Lock call from this call site, i.e. you.

It's true that there was code ifdef DEBUG to use mJSRuntime, but it also null checked mJSRuntime before using it.
Comment 1 timeless 2009-06-07 01:06:43 PDT
Created attachment 382002 [details] [diff] [review]
only create mWatchdogThread if mWatchdogWakeup is around to release it
Comment 2 Brendan Eich [:brendan] 2009-06-09 11:34:12 PDT
Timeless, thanks -- looks like an honest mistake. Should try to fix soon, maybe for 1.9.1. The bug you cite, bug 496628, was marked INVALID, blamed on a distro. But if this bug was the cause, then we should probably change that bug's status retroactively. We may not be able to prove what caused that bug, though.

/be
Comment 3 Andreas Gal :gal 2009-06-09 11:39:12 PDT
Comment on attachment 382002 [details] [diff] [review]
only create mWatchdogThread if mWatchdogWakeup is around to release it

Ben, can you review?
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-06-09 15:41:21 PDT
Comment on attachment 382002 [details] [diff] [review]
only create mWatchdogThread if mWatchdogWakeup is around to release it

This looks fine.
Comment 6 Mike Beltzner [:beltzner, not reading bugmail] 2009-08-25 10:38:48 PDT
Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)
Comment 7 Ted Mielczarek [:ted.mielczarek] 2009-12-04 04:49:44 PST
This didn't make it back to 1.9.1, did it? I was poking around some crash reports and saw a 3.5.4 crash here:
http://crash-stats.mozilla.com/report/index/f5cd0d58-932b-4448-8072-1ff432091203
Comment 8 timeless 2009-12-05 08:53:13 PST
*** Bug 496628 has been marked as a duplicate of this bug. ***
Comment 9 Daniel Veditz [:dveditz] 2009-12-21 15:31:12 PST
Comment on attachment 382002 [details] [diff] [review]
only create mWatchdogThread if mWatchdogWakeup is around to release it

Approved for 1.9.1.8, a=dveditz for release-drivers
Comment 10 Ted Mielczarek [:ted.mielczarek] 2009-12-22 03:58:29 PST
Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/15fb9a1c8f71
Comment 11 Henrik Skupin (:whimboo) 2010-01-27 17:46:58 PST
Timeless, is there any reliable way for QA to test this fix? Having a stack we can query on to determine no more crashes happen?
Comment 12 timeless 2010-01-28 00:07:15 PST
there's going to be a different signature for each platform...

*IF* pthread_mutex_lock, RtlpWaitForCriticalSection and RtlEnterCriticalSection are added to some ignore list, then instead of [@ RtlpWaitForCriticalSection | RtlEnterCriticalSection ] we could look for [@ PR_Lock | XPCJSRuntime::XPCJSRuntime] with crash address 0x10

this is based on ted's crash report which clearly is fixed by this bug:
UUID	f5cd0d58-932b-4448-8072-1ff432091203
Crash Address	0x10
0 	ntdll.dll 	RtlpWaitForCriticalSection 	
1 	ntdll.dll 	RtlEnterCriticalSection 	
2 	nspr4.dll 	PR_Lock
3 	xul.dll 	XPCJSRuntime::XPCJSRuntime

I'm still not quite sure about the Linux crash, the interesting pointer was NO_SCOPE_SHARING_TODO (lock=0xfeedbeef), which doesn't make much sense. Brendan would have to explain other conditions under which we'd get that value. It's possible that an optimizer confused the debugger and lock really is 0x0.
Comment 13 Henrik Skupin (:whimboo) 2010-01-29 07:15:14 PST
We have a lot of those PRLOCK signatures on crashstat. See the link below. But while checking some of the crashes none has the same stack as given by Ted. So we should probably wait until 3.5.8 has been released and monitor the crash stats a couple of days after the release. Without a testcase I do not want to mark it as verified on 1.9.1. Thanks timeless!

http://crash-stats.mozilla.com/report/list?query_search=signature&query_type=exact&query=PR_Lock&date=&range_value=1&range_unit=weeks&process_type=all&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&signature=PR_Lock

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