Dave Townsend [:mossop] Assignee
	Description • 16 years ago

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

	Daniel Veditz [:dveditz]
	Comment 1 • 16 years ago

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?

	Dave Townsend [:mossop] Assignee
	Comment 2 • 16 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 3 • 16 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 4 • 16 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 5 • 16 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 6 • 16 years ago

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

r/sr=dveditz

	Dave Townsend [:mossop] Assignee
	Comment 7 • 16 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 8 • 16 years ago

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

	Samuel Sidler (old account; do not CC)
	Comment 9 • 16 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 10 • 16 years ago

(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

	Samuel Sidler (old account; do not CC)
	Comment 11 • 16 years ago

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.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 12 • 16 years ago

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

a191=beltzner

	Dave Townsend [:mossop] Assignee
	Comment 13 • 16 years ago

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

	Samuel Sidler (old account; do not CC)
	Comment 14 • 16 years ago

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.

	Dave Townsend [:mossop] Assignee
	Comment 15 • 16 years ago

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

	Al Billings [:abillings - ex-MoCo]
	Comment 16 • 16 years ago

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.