Last Comment Bug 896124 - Implement a JS_Init JSAPI function that must be called before any JSAPI entrypoint is called
: Implement a JS_Init JSAPI function that must be called before any JSAPI entry...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla25
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
: 896123 (view as bug list)
Depends on: 900026 901076 992641 899270 900406
Blocks: 890127
  Show dependency treegraph
 
Reported: 2013-07-19 17:23 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2014-04-06 04:28 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (13.72 KB, patch)
2013-07-19 17:26 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bobbyholley: review-
Details | Diff | Splinter Review
Init before components manager init, shutdown after components manager shutdown (14.45 KB, patch)
2013-07-22 11:16 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
benjamin: review+
luke: review+
Details | Diff | Splinter Review
2 - Move ICU memory reporting management to nsXPComInit.cpp so u_setMemoryFunctions precedes u_init (6.53 KB, patch)
2013-07-25 14:49 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
benjamin: review+
Details | Diff | Splinter Review
Delta atop first patch, to make threadsafe Windows work (6.44 KB, patch)
2013-07-26 15:27 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-19 17:23:54 PDT
ICU provides initialization and cleanup functions that must be called for memory allocations and the like to be tracked correctly.  The latter in particular is relevant for tinderbox tracking of allocations and leaks -- if we don't cleanup ICU, we effectively leak whatever it's allocated under the hook, that wasn't allocated because it was needed for some particular ICU object we allocated and properly deallocated.

We already have JS_ShutDown for the cleanup hook, and somewhat surprisingly it seems to be called in the places that matter already.  All that's needed is an identical init function.

I'll add the ICU init/cleanup function calls in a separate bug; this is just to carve out the JSAPI space for it, and to clear out JS_NewRuntime of a whole bunch of init codee that really belongs in JS_Init.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-19 17:26:27 PDT
Created attachment 778745 [details] [diff] [review]
Patch

jsapi-tests, jstests, jit-tests pass.  Browser starting up, browsing around, viewing a web workers demo, etc. seems to work.  mach xpcshell-tests runs a few tests correctly.  Judging by the places that call JS_ShutDown(), this should be good for review.  Given what this does, I'll kick off a try run for confirmation of sanity, of course.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-19 17:28:36 PDT
Comment on attachment 778745 [details] [diff] [review]
Patch

I probably should push the xpconnect bits off onto an xpconnect person.  :-)

The rest of it is fairly simple JSAPI stuff, although Luke might have to read a little code to figure out that the init stuff in JS_Init is actually repeatable without harm.  The jsapi-tests should do the trick for testing that mode of interaction.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-19 17:40:13 PDT
*** Bug 896123 has been marked as a duplicate of this bug. ***
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2013-07-19 18:11:07 PDT
Comment on attachment 778745 [details] [diff] [review]
Patch

Review of attachment 778745 [details] [diff] [review]:
-----------------------------------------------------------------

As noted on IRC, there will soon be multiple CycleCollectedJSRuntimes, so this won't work. I think we should put this in XPCOM startup and shutdown, and get bsmedberg's r+.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-22 11:16:04 PDT
Created attachment 779292 [details] [diff] [review]
Init before components manager init, shutdown after components manager shutdown

bsmedberg, if I want to add a JSAPI method to start up the engine, for performing engine-wide tasks, it sounds like the right place to do that is in XPCOM startup/shutdown.  Are these placements of the calls (in xpcom/build/nsXPComInit.cpp) correct?  The current location of the JS_ShutDown call is just wrong, if we're going to have multiple CycleCollectedRuntimes soon.  No need to review anything else here, but feel free to look at the js/src/jsapi.h changes if you want to know the init/shutdown methods' semantics.

https://tbpl.mozilla.org/?tree=Try&rev=ab806d6246d4
Comment 6 Benjamin Smedberg [:bsmedberg] 2013-07-22 12:08:14 PDT
Comment on attachment 779292 [details] [diff] [review]
Init before components manager init, shutdown after components manager shutdown

Typically, the xpconnect code has been responsible for managing the JS engine. I think it would be preferable for XPConnect to call JS_Init and JS_ShutDown (not JS_Shutdown?), but I don't have a strong preference.

Can JS_Init really fail in any normal configuration? Can we just make it infallible?
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2013-07-22 12:55:44 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Comment on attachment 779292 [details] [diff] [review]
> Init before components manager init, shutdown after components manager
> shutdown
> 
> Typically, the xpconnect code has been responsible for managing the JS
> engine.

Well, XPConnect manages the XPConnect runtime. It has nothing to do with other runtimes, like works or the ProxyAutoConfig runtime. This is why I argue against this happening in XPConnect.

> I think it would be preferable for XPConnect to call JS_Init and
> JS_ShutDown (not JS_Shutdown?), but I don't have a strong preference.

But then we have to make all non-XPConnect JS stuff depend on the XPConnect lifetime, even on other threads where we hard abort if you try to even see if XPConnect is alive.

> 
> Can JS_Init really fail in any normal configuration? Can we just make it
> infallible?
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-22 13:20:17 PDT
I have to admit I don't much care how JS_ShutDown is spelled.  :-)  I just preserved the existing spelling.

It's standard operating procedure for JSAPI methods to be fallible whenever they might fail, even if it's unlikely they'd fail.  I don't think this bug is the place to change that, nor to make a one-off exception.
Comment 9 Benjamin Smedberg [:bsmedberg] 2013-07-22 13:24:46 PDT
ok, given that, I guess that doing it in NS_InitXPCOM is ok. Please NS_RUNTIMEABORT("JS_Init failed") instead of propagating the error.
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-25 14:49:51 PDT
Created attachment 781235 [details] [diff] [review]
2 - Move ICU memory reporting management to nsXPComInit.cpp so u_setMemoryFunctions precedes u_init

Bug 890238 bitrotted this by adding a JS_SetICUMemoryFunctions API, which calls u_setMemoryFunctions.  That function *must* be called before u_init per ICU docs.  That means all the ICU memory stuff has to move to nsXPComInit.cpp so that it can happen before JS_Init.  This patch performs that move, as a delta atop the original patch here.

As I note in the comments, it'd be better if JS_Init did all this and we just exposed a |size_t JS_GetICUMemoryUsage()| API.  But alloc-function memory reporting depends on mozalloc, which the JS engine doesn't depend on, so we can't move this junk there right now.  :-\
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-25 17:56:42 PDT
Part 1 is landable without part 2 -- part 2's only necessary if you want to use the patch from bug 890127, and are building with Intl enabled -- so I landed it:

https://hg.mozilla.org/integration/mozilla-inbound/rev/631b3d5d54f4

Leaving open to take care of part 2 still...
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-07-25 22:36:28 PDT
Something in this push made Windows checktests mad. Backed out per our IRC conversation.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbf37166d07c
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-26 02:29:57 PDT
I did a Windows build, and the jsapi-test failure implicates PRMJ_ShutdownNow() code, clearly this bug.  I'll look a bit more after some sleep and figure out what's up.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-26 15:27:59 PDT
Created attachment 781975 [details] [diff] [review]
Delta atop first patch, to make threadsafe Windows work

The PR_CallOnce(&calibrationOnce, NowInit) in PRMJ_Now() is the problem.  In a world where JS_Init and JS_ShutDown can be called multiple times, call-once semantics must become call-once-per-init.  Unfortunately NowInit is slow (15ms, but we want to start up without waiting on it), so we can't just call it from JS_Init.

It'd take more time than it's worth now to convert call-once to call-once-per-init, so I'm punting on the ability to init the JS engine multiple times.  The API requirement will be: call JS_Init, then do JSAPI stuff, then call JS_ShutDown, and then you can't use the JS engine *at all* any more.

This delta patch moves the init/shutdown calls in jsapi-tests so they only happen once, changes the jsInitialized bool to an enum of uninitialized/running/shutdown for more precise state-checking assertions, and updates documentation.  It should fix the Windows issues, but we'll see what a try run says.  If that says yes, I'm going to go ahead with this -- these changes are all straightforward enough.

https://tbpl.mozilla.org/?tree=Try&rev=c9f9fa7836f0
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-27 21:15:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e432690bf0fb
Comment 16 Ed Morley [:emorley] 2013-07-29 08:23:04 PDT
https://hg.mozilla.org/mozilla-central/rev/e432690bf0fb
Comment 17 Mike de Boer [:mikedeboer] 2013-07-30 11:58:58 PDT
Waldo, I consistently get the following error message + crash when I run a debug build of m-c re-winded to e432690bf0fb:

...
WARNING: NS_ENSURE_TRUE(startupCache) failed: file /Users/mdeboer/Projects/mozilla-central/content/xbl/src/nsXBLDocumentInfo.cpp, line 676
--DOCSHELL 0x10051ec00 == 0 [id = 1]
WARNING: nsExceptionService ignoring thread destruction after shutdown: file /Users/mdeboer/Projects/mozilla-central/xpcom/base/nsExceptionService.cpp, line 168
WARNING: Leaking the RDF Service.: file /Users/mdeboer/Projects/mozilla-central/rdf/build/nsRDFModule.cpp, line 165
Assertion failure: !JSRuntime::hasLiveRuntimes() (forgot to destroy a runtime before shutting down), at /Users/mdeboer/Projects/mozilla-central/js/src/jsapi.cpp:718

^-- that's the last warning I see and then... boom!

My env: 2.7GHz i7 MbPro, retina, 16GB RAM, OSX 10.8.4
.mozconfig: ac_add_options --enable-debug

Updating to the commit before this one (3bc8a3f77169) and building it boots the Nightly Debug fine, without the warning mentioned above.

If I can provide you with more info, please let me know!
Comment 18 Mike de Boer [:mikedeboer] 2013-07-30 12:00:01 PDT
Forgot to mention: the crash persists when I update to tip and rebuild. Clobbering has no positive effect.
Comment 19 Mike de Boer [:mikedeboer] 2013-07-31 03:12:36 PDT
I'm going to be bold and re-open this bug. I truly cannot use an up-to-date m-c at this point, which I believe should be resolved.
Comment 20 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-31 12:46:44 PDT
What extensions are you using?  It's not impossible that some extension is causing a JSRuntime to be leaked, and if you leak a JSRuntime past XPCOM shutdown, you'll hit that assertion.  Or it could be something in your profile.
Comment 21 Mike de Boer [:mikedeboer] 2013-07-31 12:53:13 PDT
No extensions... I found out it's like I mentioned in bug 899270, the Profile Manager. Could be it leaks like a sieve, not sure. Might be it's worth not making it an assertion on shutdown to circumvent that.
Comment 22 Marco Bonardo [::mak] 2013-08-01 06:06:55 PDT
confirmed here, the profile manager triggers the assertion, so it's impossible to use a debug build if not by passing -p profilename to it.
Comment 23 Andrew McCreight [:mccr8] 2013-08-02 10:52:59 PDT
Yes, the profile manager always leaks on shutdown (bug 732493), and this bug turns at least some shutdown leaks into a fatal assertion.  I morphed bug 899270 into fixing this problem.  For now, I recommended just commenting out this assertion...
Comment 24 Marco Bonardo [::mak] 2013-08-02 11:50:19 PDT
(In reply to Andrew McCreight [:mccr8] from comment #23)
> For now, I recommended just commenting out
> this assertion...

I don't think that's a good solution, unless it's matter of a couple days to fix the cause.
Just think of new contributors, their first build will immediately crash on startup due to the assertion. It may also cause other developers to waste a bunch of time trying to figure why their change is causing an assertion on startup.
Couldn't we just convert the MOZ_ASSERT to a WARNING until the underlying issue is fixed?
Comment 25 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-02 13:03:45 PDT
The assertion is inside the JS engine.  There's no warning mechanism we could use in the interim.  Maybe printf, if we give up and let the terrorists win.  ;-)

Call me cynical (ha!), but turning this into a warning is definitely not going to cause it to be fixed.

I'm going to close this again and open up a new bug for the profile manager issue, that doesn't spam everyone watching JS engine issues.
Comment 26 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-02 14:07:39 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/1b9a459cea77

You may now return to your regularly scheduled ignoring of any invariant the violation of which is not fatal.  :-\
Comment 27 Andrew McCreight [:mccr8] 2013-08-02 14:09:15 PDT
Every time I've seen this assertion, we were leaking anyways, so it'll still turn the tree orange.  Thanks for fixing this!
Comment 29 Carsten Book [:Tomcat] - PTO-back Sept 4th 2013-08-05 03:16:46 PDT
https://hg.mozilla.org/mozilla-central/rev/1b9a459cea77

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