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

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: charlieb, Assigned: dveditz)

Tracking

({crash, fixed1.8.1.8})

1.5.0.x Branch
x86
Windows XP
crash, fixed1.8.1.8
Points:
---
Bug Flags:
blocking1.8.1.8 +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
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

12 years ago
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)

Updated

12 years ago
Assignee: nobody → dveditz
Keywords: crash
(Assignee)

Updated

12 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

12 years ago
Created attachment 263077 [details] [diff] [review]
Make sure triggers are deleted before the context

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

12 years ago
Whiteboard: need r=jst
(Assignee)

Comment 4

12 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

12 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?
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

12 years ago
Whiteboard: need r=jst → dveditz see comment 5, need r=jst
(Assignee)

Updated

12 years ago
Attachment #263077 - Flags: approval1.8.1.5?
Attachment #263077 - Flags: approval1.8.0.13?
(Assignee)

Updated

12 years ago
Whiteboard: dveditz see comment 5, need r=jst
(Assignee)

Updated

12 years ago
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
(Assignee)

Updated

12 years ago
Flags: blocking1.8.0.13+ → blocking1.8.0.14?
(Assignee)

Comment 6

11 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.
Attachment #263077 - Flags: review?(jst) → review+
(Assignee)

Comment 7

11 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 8

11 years ago
Fix checked into the 1.8 branch
Keywords: fixed1.8.1.8
(Assignee)

Comment 9

11 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?
Attachment #263077 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 10

11 years ago
fix checked into trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
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.