71237 - Prefs engine needs to keep ref to JSRuntimeService.

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect, P2)

Description

[Randell Jesup [:jesup] (needinfo me)]

FreeBSD 4.1 20010306xx pull/build

Closed the mozilla window.  Crashed apparently due to a JS lock having been
already freed.  See attachment.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: backtrace and structure dumps

Comment 2

[Phil Schwartau]

I'm guessing from the stack trace that this should go to the Preferences
component. Reassignig. cc'ing Brendan and jband in case I'm wrong about this.  
This stack trace is one I've never seen before. 


Randell, does this happen every time you close the Mozilla window? 
Or are there any specific steps leading up to this that are necessary
to reproduce the crash?   

Comment 3

[Phil Schwartau]

cc'ing self - 

Comment 4

[John Bandhauer]

Yes, this is a prefs bug. It is destroying a JSContext after the JSRuntime has 
been destroyed. The prefs service should be holding a reference to the JSRuntime 
service to keep it from shutting down before the prefs service is done with the 
JSRuntime.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I'll take this.

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Sadly, it doesn't look like people have to keep the prefs service alive the
whole time either.  Could get ugly in here.

Comment 7

[John Bandhauer]

Better to leak a JSContext at shutdown then crash. Prefs needs to at least 
*ask* for the JS runtime service before whacking the JSContext. It can let the 
JSContext leak if the service manager refused to give up the runtime service 
(i.e. it has *already* leaked).

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

But that doesn't help us in the case where we bounce off of zero refs and
restart the service, does it?

I guess I could check that the new runtime == cx->runtime, and just hope that we
didn't get a new reallocation in the same spot.

Someone's going to rewrite all of prefs, right?

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: patch to attempt to get the runtime service

Comment 10

[John Bandhauer]

We never bounce off of zero refs and restart the service. The serive makes an
extra reference to itself that is destoryed in its module shutdown. The service
manager is also holding a ref. Module shutdown happens only in service manager
shutdown - during and after which time the service manager will refuse requests.
So, the service will neither prematurely die nor be brought back to life (with a
new JSRuntime!). Any violation of these rules is a bug to be fixed without delay.

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

So should I take that rt == check out, or will you give me review love for
what's posted?

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: better patch (also for 39373)

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This patch makes prefs compile warning-free, removes a particularily egregious
piece of classic-era code (FE_Confirm?  _JSDecoder_?), sprinkles requests around
and actually compiles.  Also makes a mean cup of coffee.

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: even better patch!

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

And _this_ patch doesn't crash at startup because of a braino.

I see the runtime-mismatch warning at shutdown, so I think we're really averting
a crash here.  If the code paths into PREF_CleanupPrefs (including the one from
PREF_PrefCleanup!) weren't so incredibly many and twisted, I'd fix this routine
up to take a JSRuntime * and only destroy the context if we have one, but since
bnesse is rewriting this stuff _anyway_, I think this bandaid will get us through.

Do we want to take this for 0.8.1?

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Moving this back to 0.8.1, now that it's on the drivers' radar.

Comment 17

[Brendan Eich [:brendan]]

Looks good to me, r/sr=[e-mail].

I'm filing a spin-off bug 72552 about excessive CHECK_REQUEST macro calls in
jsapi.c (the one in JS_SetErrorReporter will bite this bug's patch).

/be

Comment 18

[Alec Flett]

I guess this looks ok, but I don't understand the JS_Begin/EndRequest stuff...
why do we need it?

Comment 19

[Brendan Eich [:brendan]]

The JS engine, when compiled with JS_THREADSAFE defined and used in a
multi-threaded embedding, requires users to bracket maximal non-blocking batches
of API calls and intermingled native code with Requests.  These serve two needs
in the current engine:

(1) Requests prevent the JS GC from racing badly with engine code in the API
entry points and in the JS compiler where unrooted atoms and objects are held by
weak (C) refs.  The embedding is responsible for calling the GC often enough,
but it need not worry about races if it brackets API calls with requests.

(2) Requests help avoid *any* locking overhead for objects (scopes, actually)
that are used by at most one request-thread at a time.  This lock-free scope
synchronization feature is a huge win in terms of thread-safe JS performance: in
common cases, JS runs as fast MT as ST. See bug 54743.

/be

Comment 20

[Alec Flett]

ok, if you say so.. I still maintain that prefs should not be used in a
multi-threaded way, but if there's no performance loss, then s/r=alecf. 

adding bnesse so that when he updates his tree he doesn't wonder what the heck
this is for

Comment 21

[Brendan Eich [:brendan]]

alecf: you can maintain what should be, but there are no runtime checks or even
assertions (right?) to the contrary, and the plugin API in particular seems
likely to lead to control flow on several threads reaching the prefs code.  If
prefs is getting a rewrite, will it be thread-safe?  If not, it had better
insist that it's running only on the main thread.

/be

Comment 22

[Alec Flett]

true enough. but the prefs API itself is not being exposed to plugins, at least
I hope not - the prefs API should only be exposed to the capabilities library,
which should ensure that it's accessing prefs from the right thread... the only
people accessing the prefs API should be mozilla itself and embedding, and so
embedding is the only consumer which should know about the thread-unsafeness.

Comment 23

[Alec Flett]

oops, that meant 'r=alecf'

Comment 24

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Thanks, all.  It's in on the branch, so I'm moving the milestone and waiting to
hit the trunk (if and when we see some green).

Comment 25

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This patch, provided by dbaron after mad minutes trying to figure out why I
busted prefs on the branch, fixes evil.  Turns out that PREF_Init is being
called multiple times, and the previous patch had us failing (falsely) on every
call after the first.

My bad.  Thanks, David.

Index: prefapi.c
===================================================================
RCS file: /cvsroot/mozilla/modules/libpref/src/prefapi.c,v
retrieving revision 3.86.8.1
diff -u -d -u -1 -r3.86.8.1 prefapi.c
--- prefapi.c	2001/03/20 01:14:12	3.86.8.1
+++ prefapi.c	2001/03/20 04:07:05
@@ -274,3 +274,3 @@
 {
-    PRBool ok = PR_FALSE, request = PR_FALSE;
+    PRBool ok = PR_TRUE, request = PR_FALSE;
     extern JSRuntime* PREF_GetJSRuntime(void);
@@ -289,2 +289,3 @@
     {
+        ok = PR_FALSE;
         gMochaContext = JS_NewContext(gMochaTaskState, 8192);

Comment 26

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

In the tip.

Comment 27

[sairuh (rarely reading bugmail)]

rubberstamp.