Closed Bug 896124 Opened 11 years ago Closed 11 years ago

Implement a JS_Init JSAPI function that must be called before any JSAPI entrypoint is called

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: Waldo, Assigned: Waldo)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 1 obsolete file)

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.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #778745 - Flags: review?(luke)
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.
Attachment #778745 - Flags: review?(bobbyholley+bmo)
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+.
Attachment #778745 - Flags: review?(bobbyholley+bmo) → review-
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
Attachment #778745 - Attachment is obsolete: true
Attachment #778745 - Flags: review?(luke)
Attachment #779292 - Flags: review?(benjamin)
Attachment #779292 - Flags: review?(luke)
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?
(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?
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.
ok, given that, I guess that doing it in NS_InitXPCOM is ok. Please NS_RUNTIMEABORT("JS_Init failed") instead of propagating the error.
Attachment #779292 - Flags: review?(benjamin) → review+
Attachment #779292 - Flags: review?(luke) → review+
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.  :-\
Attachment #781235 - Flags: review?(benjamin)
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...
Whiteboard: [leave open]
Something in this push made Windows checktests mad. Backed out per our IRC conversation.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbf37166d07c
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.
Attachment #781235 - Flags: review?(benjamin) → review+
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/e432690bf0fb
Whiteboard: [leave open]
Target Milestone: --- → mozilla25
https://hg.mozilla.org/mozilla-central/rev/e432690bf0fb
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 899270
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!
Flags: needinfo?(jwalden+bmo)
Forgot to mention: the crash persists when I update to tip and rebuild. Clobbering has no positive effect.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(jwalden+bmo)
Depends on: 900026
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.
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.
Depends on: 900406
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.
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...
(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?
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.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 901076
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.  :-\
Every time I've seen this assertion, we were leaking anyways, so it'll still turn the tree orange.  Thanks for fixing this!
Depends on: 992641
You need to log in before you can comment on or make changes to this bug.