374470 - InstallTrigger callback function possibly called on wrong thread

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Dave Townsend [:mossop]]

While debugging I discovered the following assertion while xpinstall was attempting to notify the callback function after installing the extension from the given page. It suggests to me that the page's javascript may be getting called on the wrong thread which I guess is a bad thing.

Assertion failure: cx->thread->id == js_CurrentThreadId(), at /Users/dave/mozilla/source/HEAD/mozilla/js/src/jsapi.c:829

#0	0x010d2d64 in JS_Assert at jsutil.c:63
#1	0x01015441 in JS_BeginRequest at jsapi.c:829
#2	0x3c22ad3e in nsXPITriggerInfo::SendStatus at nsXPITriggerInfo.cpp:344
#3	0x3c22e878 in nsXPInstallManager::OnInstallDone at nsXPInstallManager.cpp:1210
#4	0x3c227123 in nsTopProgressListener::OnInstallDone at nsTopProgressNotifier.cpp:189
#5	0x3c21f3c1 in RunInstallOnThread at nsSoftwareUpdateRun.cpp:580
#6	0x005a3943 in _pt_root at ptthread.c:220
#7	0x90024147 in _pthread_body

Comment 2

[Dave Townsend [:mossop]]

When this asserts it stops proper notification of the xpi download completion to get to the extension manager.

Comment 3

[Dave Townsend [:mossop]]

To clarify, the callback is called on the correct thread. The assertion comes from shortly before the callback is called where the callback is added as a js root to prevent GC

Comment 4

[Robert Kaiser]

I hit this when trying to install Nightly Tester Tools on a current SeaMonkey trunk debug build (Linux) and did some investigation in gdb with timeless' help.
Here are some relevant bits from IRC:

<timeless> looks like someone needs to call JS_SetContextThread
<timeless> basically probably each place that does JS_BeginRequest for the first time near a thread probably needs to do JS_SetContextThread
<KaiRo> oh, I'm glad I could even find out the info, I don't think I could fix code I don't really understand in the first place ;-)
<timeless> the code right now does:
<timeless> JS_BeginRequest(cx);
<timeless> it'll need to say:
<timeless> JS_SetContextThread(cx);
<timeless> JS_BeginRequest(cx);
<timeless> you're litterally adding 1 repetitive line to <10 places in xpinstall
<timeless> (and testing:)
<timeless> http://mxr.landfill.bugzilla.org/seamonkey/ident?i=JS_BeginRequest&tree=seamonkey&filter=xpinstall
<timeless> so it's more like 7 cases
<timeless> note that places w/ JS_NewContext don'nt need it
<timeless> so, http://mxr.landfill.bugzilla.org/seamonkey/source/xpinstall/src/nsSoftwareUpdateRun.cpp is fine
<timeless> afaict every instance of it in xpitriggerinfo needs it

Comment 5

[timeless]

Attachment: random thoughts

this is a start, try it out? :)

Comment 6

[timeless]

Comment on attachment 269679 [details] [diff] [review]
random thoughts

afaict, there's no useful way to use JS_GetContextThread short of creating another CX or stashing the correct answer in a member variable, which is fairly annoying. if dveditz wants, i can do the latter....

this is of course untested (and it isn't even a properly generated cvs diff...)

Comment 7

[Robert Kaiser]

Comment on attachment 269679 [details] [diff] [review]
random thoughts

Unfortunately, this makes us segfault in jsapi.c:829 - I'll attach the stack form that segfault per timeless' request.

Comment 8

[Robert Kaiser]

Attachment: stacktrace from segfault with "random thoughts" patch

Some IRC comments of timeless about what is going wrong there:

<timeless> basically what we did was>
<timeless> 1. steal a jscontext
<timeless> 2. use it on a different thread
<timeless> 3. tell it that no one was using it and prayed that whomever would use it next would claim it first
<timeless> what this stack shows is that someone else (main thread) uses it
<timeless> i think the right thing to do is just make another jscontext for the other thread

<timeless> and i do mean steal btw, what we (xpinstall) were doing was very impollite
<timeless> kairo, http://developer.mozilla.org/en/docs/JS_NewContext
<KaiRo> timeless: so I guess "...so long as it's associated via JS_SetContextThread with only one thread at a time." is what is our/your problem there, right?
<timeless> kairo: yeah
<timeless> the assert crashed because it was asserted w/ 0
<timeless> it used to fail(abort) because it was associated w/ a *different* 1

Comment 9

[Dave Townsend [:mossop]]

No more threading in xpinstall so this is fixed by bug 406807