Closed
Bug 368811
Opened 18 years ago
Closed 17 years ago
Firefox crashes on exit after plugin install activated by InstallTrigger.install(xpi,installCompleteFunction)
Categories
(Firefox :: General, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: charlieb, Assigned: dveditz)
Details
(Keywords: crash, fixed1.8.1.8)
Attachments
(1 file)
2.89 KB,
patch
|
jst
:
review+
dveditz
:
approval1.8.1.8+
mconnor
:
approval1.9+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
After installing a plugin using xpinstall, and triggering the install with InstallTrigger.install(xpi,installCompleteFunction), the browser crashes on exit in JS3250.dll if the Add-ons window (which pops up during plugin install) is closed after the last page.
I built a debug build of Firefox 1.5.0.9 from sources and the crash happens in
JS3250.ddl:
nsXPITriggerInfo::~nsXPITriggerInfo()
{
nsXPITriggerItem* item;
for(PRUint32 i=0; i < Size(); i++)
{
item = Get(i);
if (item)
delete item;
}
mItems.Clear();
if ( mCx && !JSVAL_IS_NULL(mCbval) ) {
JS_BeginRequest(mCx);
>>>> JS_RemoveRoot( mCx, &mCbval );
JS_EndRequest(mCx);
}
MOZ_COUNT_DTOR(nsXPITriggerInfo);
}
After some debugging, I verified that during the install, the nsXPITriggerInfo receives the pointer to the callback function installCompleteFunction in the SaveCallback() method, which saves it to the mCbval variable.
When install is completed, the callback is called and then the method
static void destroyTriggerEvent(XPITriggerEvent* event)
{
JS_BeginRequest(event->cx);
JS_RemoveRoot( event->cx, &event->cbval );
JS_EndRequest(event->cx);
delete event;
}
is correcly executed.
Then, when the last page has been closed and the add-on window has been closed, the destructor is called and crashes.
Reproducible: Always
Steps to Reproduce:
1. Open a web page that launches a plugin install with InstallTrigger.install(xpi,installCompleteFunction)
1b. The add-on window pops up and display the installation advace
2. Open a web page that uses the plugin
3. Open another page / tab
4. Close all pages
5. Close the Add-on window
Actual Results:
Crash
Expected Results:
Smooth exit
It was unable to reproduce the crash with Talkback installed.
Reporter | ||
Comment 1•18 years ago
|
||
The bug happens in both 1.5.0.9 and 2.0.0.1
Version: unspecified → 1.5.0.x Branch
Comment 2•17 years ago
|
||
Confirming. Crash occurs with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2 as well. I installed the shockwave plug-in and received this crash on exit. Talkback incident TB29709734X.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8.1.3?
Flags: blocking1.8.0.11?
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Assignee | ||
Comment 3•17 years ago
|
||
The crash is because we try to remove the callback function's root after the page context has gone away. Moving the trigger deletion into nsXPInstallManager::Shutdown()--which happens when the install finishes--works around this crash. But why is the nsXPInstallManager living until the ExtensionManager dialog goes away? It should have shut down when the page shut down, or caused the context to stick around until it did. While debugging this the JS_BeginRequest() calls for the event are happening on the wrong thread and triggering a JS_ASSERT. The threading in XPInstall has always worked like that so the asserts must have been added later (but I doubt people test much with install callback functions). The upshot is we may need additional fixes here, but this helps.
Attachment #263077 -
Flags: review?(jst)
Attachment #263077 -
Flags: approval1.8.1.4?
Attachment #263077 -
Flags: approval1.8.0.12?
Assignee | ||
Updated•17 years ago
|
Whiteboard: need r=jst
Assignee | ||
Comment 4•17 years ago
|
||
No reviews or baking, guess this misses this release.
Flags: blocking1.8.1.5+
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.13+
Flags: blocking1.8.0.12+
Assignee | ||
Updated•17 years ago
|
Attachment #263077 -
Flags: approval1.8.1.5?
Attachment #263077 -
Flags: approval1.8.1.4?
Attachment #263077 -
Flags: approval1.8.0.13?
Attachment #263077 -
Flags: approval1.8.0.12?
Comment 5•17 years ago
|
||
Dan, did you want to consider changing this code to rely on the JSContext in question being an nsIScriptContext and holding a strong reference to that while you need the context rather than this workaround? I'm happy to review this if you're still interested in landing the workaround.
Assignee | ||
Updated•17 years ago
|
Whiteboard: need r=jst → dveditz see comment 5, need r=jst
Assignee | ||
Updated•17 years ago
|
Attachment #263077 -
Flags: approval1.8.1.5?
Attachment #263077 -
Flags: approval1.8.0.13?
Assignee | ||
Updated•17 years ago
|
Whiteboard: dveditz see comment 5, need r=jst
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.8.0.13+ → blocking1.8.0.14?
Assignee | ||
Comment 6•17 years ago
|
||
Jst: I'd like to add this patch, please review it. The Extension Manager dialog _will_ hang on to the install manager as long as its open, and I don't need to keep the JS context alive that long. But I do need to clear the triggers when the install is finished, not when the Add-ons dialog is shut.
Updated•17 years ago
|
Attachment #263077 -
Flags: review?(jst) → review+
Assignee | ||
Comment 7•17 years ago
|
||
Comment on attachment 263077 [details] [diff] [review] Make sure triggers are deleted before the context approved for 1.8.1.8, a=release-drivers
Attachment #263077 -
Flags: approval1.8.1.8+
Assignee | ||
Comment 9•17 years ago
|
||
Comment on attachment 263077 [details] [diff] [review] Make sure triggers are deleted before the context Would be nice to have this safe crash fix on the trunk.
Attachment #263077 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #263077 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 10•17 years ago
|
||
fix checked into trunk.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.8.0.14? → blocking1.8.0.15?
Comment 11•16 years ago
|
||
Marking blocking 1.8.0.15+ to investigate further...
Flags: blocking1.8.0.15? → blocking1.8.0.15+
You need to log in
before you can comment on or make changes to this bug.
Description
•