475347 - Crash after closing tab that initiated an install and included a callback function [@ js_UpdateMallocCounter]

Status: RESOLVED FIXED

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

Description

[Dave Townsend [:mossop]]

If a page does InstallTrigger.install(triggers, callback) and the user closes the tab with the page in before the install is complete then we crash attempting to call the callback.

Thread 0 Crashed:
0   libmozjs.dylib                	0x0015dcda js_UpdateMallocCounter + 30 (jsgc.cpp:3839)
1   libmozjs.dylib                	0x001377f3 JS_malloc + 87 (jsapi.cpp:1867)
2   libmozjs.dylib                	0x001ad297 js_NewStringCopyZ + 51 (jsstr.cpp:2938)
3   libmozjs.dylib                	0x00138b9e JS_PushArgumentsVA + 680 (jsapi.cpp:367)
4   libmozjs.dylib                	0x00138c8f JS_PushArguments + 41 (jsapi.cpp:301)
5   XUL                           	0x02761d7a XPITriggerEvent::Run() + 74 (nsXPITriggerInfo.cpp:251)
6   XUL                           	0x029149b1 nsThread::ProcessNextEvent(int, int*) + 273 (nsThread.cpp:511)
7   XUL                           	0x028d16f7 NS_ProcessPendingEvents_P(nsIThread*, unsigned int) + 71
8   XUL                           	0x028744e2 nsBaseAppShell::NativeEventCallback() + 98 (nsBaseAppShell.cpp:122)
9   XUL                           	0x0283d90a nsAppShell::ProcessGeckoEvents(void*) + 634 (nsAppShell.mm:375)
10  com.apple.CoreFoundation      	0x938615f5 CFRunLoopRunSpecific + 3141
11  com.apple.CoreFoundation      	0x93861cd8 CFRunLoopRunInMode + 88
12  com.apple.HIToolbox           	0x91c352c0 RunCurrentEventLoopInMode + 283
13  com.apple.HIToolbox           	0x91c35012 ReceiveNextEventCommon + 175
14  com.apple.HIToolbox           	0x91c34f4d BlockUntilNextEventMatchingListInMode + 106
15  com.apple.AppKit              	0x9219fd7d _DPSNextEvent + 657
16  com.apple.AppKit              	0x9219f630 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
17  com.apple.AppKit              	0x9219866b -[NSApplication run] + 795
18  XUL                           	0x0283d0ba nsAppShell::Run() + 186 (nsAppShell.mm:693)
19  XUL                           	0x026c4767 nsAppStartup::Run() + 71 (nsAppStartup.cpp:193)
20  XUL                           	0x01e56c2f XRE_main + 9743 (nsAppRunner.cpp:3214)
21  org.mozilla.firefox           	0x000023ec main + 252 (nsBrowserApp.cpp:156)
22  org.mozilla.firefox           	0x00001c2b _start + 209
23  org.mozilla.firefox           	0x00001b59 start + 41

Comment 1

[Daniel Veditz [:dveditz]]

We used to have this crash long ago (though I can't find the bug for it), before we rooted the callback function so it wouldn't get deleted when the page was torn down. Did something in that area change?

Comment 2

[Dave Townsend [:mossop]]

If that's the case then I might be able to find a regression range for this.

Comment 3

[Dave Townsend [:mossop]]

I should note that that stack is not necessarily consistent. It seems to crash in different places randomly.

Comment 4

[Dave Townsend [:mossop]]

If rather than closing the tab the user instead navigates to a new page then there is no crash but the following error is logged:

Error: Principal of callback context is different than InstallTriggers

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Dan, don't know how you are with JS API stuff, might be worth getting mrbkap's review on this since he's been helping me through it already.

Apparently the problem is that we were retaining a strong reference to the page's global object (window) when we should be retaining a strong reference to the js context itself. This is always possible for DOM based scripts, but not for components (but that isn't an issue since InstallTrigger does not exist there). The callback function itself should keep the window object alive.

This patch switches to keeping a strong reference to the context, it also performs an additional check to make sure that xpconnect hasn't been removed from the context since that triggers an assertion if we call the callback after that point.

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 362978 [details] [diff] [review]
patch rev 1

r/sr=dveditz

Comment 7

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/15362d7b35b7

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 362978 [details] [diff] [review]
patch rev 1

This is a low risk fix that it would be good to get on the branches. It has automated testing on trunk and I hope to get the tests on the 1.9.1 branch, but probably not 1.9.0.x

Comment 9

[Samuel Sidler (old account; do not CC)]

Dave, where are the automated tests? I didn't see them in the patch or in the checkin from comment 7.

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #9)
> Dave, where are the automated tests? I didn't see them in the patch or in the
> checkin from comment 7.

They were included as part of bug 474763

Comment 11

[Samuel Sidler (old account; do not CC)]

Comment on attachment 362978 [details] [diff] [review]
patch rev 1

Dave: Please nominate this for 1.9.0.9 after it lands on 1.9.1. It's missed 1.9.0.8.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 362978 [details] [diff] [review]
patch rev 1

a191=beltzner

Comment 13

[Dave Townsend [:mossop]]

landed on the 1.9.1 branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/196833c3fe06

Comment 14

[Samuel Sidler (old account; do not CC)]

Comment on attachment 362978 [details] [diff] [review]
patch rev 1

Approved for 1.9.0.10. a=ss on the condition that you tell QA how to verify this manually since you're not porting the tests over to 1.9.0.

Comment 15

[Dave Townsend [:mossop]]

Landed with automated test on 1.9.0.x

Checking in xpinstall/src/nsXPITriggerInfo.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPITriggerInfo.cpp,v  <--  nsXPITriggerInfo.cpp
new revision: 1.42; previous revision: 1.41
done
Checking in xpinstall/src/nsXPITriggerInfo.h;
/cvsroot/mozilla/xpinstall/src/nsXPITriggerInfo.h,v  <--  nsXPITriggerInfo.h
new revision: 1.27; previous revision: 1.26
done
Checking in xpinstall/tests/Makefile.in;
/cvsroot/mozilla/xpinstall/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.2; previous revision: 1.1
done

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.11 by looking at browser_navigateaway2.js, which is the test for this bug, which is passing, since there are no manual steps.