Closed Bug 368811 Opened 14 years ago Closed 14 years ago

Firefox crashes on exit after plugin install activated by InstallTrigger.install(xpi,installCompleteFunction)

Categories

(Firefox :: General, defect)

1.5.0.x Branch
x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: charlieb, Assigned: dveditz)

Details

(Keywords: crash, fixed1.8.1.8)

Attachments

(1 file)

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.
The bug happens in both 1.5.0.9 and 2.0.0.1
Version: unspecified → 1.5.0.x Branch
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: nobody → dveditz
Keywords: crash
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
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?
Whiteboard: need r=jst
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+
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?
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.
Whiteboard: need r=jst → dveditz see comment 5, need r=jst
Attachment #263077 - Flags: approval1.8.1.5?
Attachment #263077 - Flags: approval1.8.0.13?
Whiteboard: dveditz see comment 5, need r=jst
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
Flags: blocking1.8.0.13+ → blocking1.8.0.14?
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.
Attachment #263077 - Flags: review?(jst) → review+
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+
Fix checked into the 1.8 branch
Keywords: fixed1.8.1.8
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?
Attachment #263077 - Flags: approval1.9? → approval1.9+
fix checked into trunk.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: blocking1.8.0.14? → blocking1.8.0.15?
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.