344121 - Restart after extension update check fails [@ nsContentUtils::IsCallerChrome]

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Zach Lipton [:zach]]

STR - 
1. Using a 1.5.x profile, install one or more extensions not compatible with 2.0 (i.e. Live HTTP Headers from http://livehttpheaders.mozdev.org/)
2. Launch Bon Echo to upgrade your profile
3. When the Extension Update window opens up, choose "Don't Check" for updates

Instead of disabling the incompatible extensions and launching normally, the browser quits. On second launch, startup proceeds normally. 

This appears to be mac only. I can reproduce it dependably on Mac, but the issue does not occur on Windows.

Comment 1

[Robert Strong (they/them - no direct email)]

It does this on all OS's and it is just noticeable on Mac OS X because of the dock and on upgrade we always perform a restart due to there being several extension install / upgrade scenarios that have to be accounted for.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Just to clarify: Zach: by "on second launch, startup proceeds normally", do you mean that it restarts automatically, or do you mean "if I launch it again myself, it works"? Sounds like this bug is describing the restart not working correctly, not the restart itself.

Comment 3

[Zach Lipton [:zach]]

Just talked to Rob and I'll try to clarify things a bit. What this bug is about is that the restart doesn't happen successfully: the browser quits to restart, and does not come back up again. After manually relaunching the app, it works fine.  

Comment 4

[Zach Lipton [:zach]]

After further investigation, the restart fails to happen whether or not we check for extension updates. 

Furthermore, it turns out that we actually crash when we're trying to restart; talkback isn't triggered, but OS X generates a CrashReporter log. Here's the stack:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_PROTECTION_FAILURE (0x0002) at 0x00000000

Thread 0 Crashed:
0   org.mozilla.firefox 	0x0021c0fc nsContentUtils::IsCallerChrome() + 32
1   org.mozilla.firefox 	0x001505b0 PresShell::HandleEventInternal(nsEvent*, nsIView*, unsigned, nsEventStatus*) + 508
2   org.mozilla.firefox 	0x001502a0 PresShell::HandleEvent(nsIView*, nsGUIEvent*, nsEventStatus*, int, int&) + 1300
3   org.mozilla.firefox 	0x00216698 nsViewManager::HandleEvent(nsView*, nsGUIEvent*, int) + 236
4   org.mozilla.firefox 	0x00215a88 nsViewManager::DispatchEvent(nsGUIEvent*, nsEventStatus*) + 3356
5   org.mozilla.firefox 	0x00244afc nsGlobalWindow::Deactivate() + 348
6   org.mozilla.firefox 	0x0046d9bc nsWebShellWindow::HandleEvent(nsGUIEvent*) + 816
7   org.mozilla.firefox 	0x006dc0d8 nsWindow::DispatchEvent(nsGUIEvent*, nsEventStatus&) + 172
8   org.mozilla.firefox 	0x006dc164 nsWindow::DispatchWindowEvent(nsGUIEvent&) + 32
9   org.mozilla.firefox 	0x006d64c8 nsMacEventDispatchHandler::DispatchGuiEvent(nsWindow*, unsigned) + 112
10  org.mozilla.firefox 	0x006d67e8 nsMacEventDispatchHandler::SetDeactivated(nsWindow*) + 232
11  org.mozilla.firefox 	0x006d7d98 nsMacEventHandler::HandleActivateEvent(OpaqueEventRef*) + 352
12  org.mozilla.firefox 	0x00326478 nsMacWindow::WindowEventHandler(OpaqueEventHandlerCallRef*, OpaqueEventRef*, void*) + 540
13  com.apple.HIToolbox 	0x931ea554 DispatchEventToHandlers(EventTargetRec*, OpaqueEventRef*, HandlerCallRec*) + 692
14  com.apple.HIToolbox 	0x931e9cac SendEventToEventTargetInternal(OpaqueEventRef*, OpaqueEventTargetRef*, HandlerCallRec*) + 372
15  com.apple.HIToolbox 	0x931e9b28 SendEventToEventTargetWithOptions + 40
16  com.apple.HIToolbox 	0x9322e848 PostActivateEvent + 340
17  com.apple.HIToolbox 	0x9322dd6c HiliteAndActivateWindow + 360
18  com.apple.HIToolbox 	0x9322daf4 AdjustToNewWindowActivation(WindowData*, WindowContext*, OpaqueWindowPtr*, WindowData*) + 156
19  com.apple.HIToolbox 	0x932fbb74 HideWindow + 192
20  org.mozilla.firefox 	0x00326920 nsMacWindow::Show(int) + 1096
21  org.mozilla.firefox 	0x0046fc78 nsXULWindow::Destroy() + 284
22  org.mozilla.firefox 	0x0046e93c nsWebShellWindow::Destroy() + 280
23  org.mozilla.firefox 	0x000826d8 nsAppShellService::Observe(nsISupports*, char const*, unsigned short const*) + 64
24  libxpcom_core.dylib 	0x2c00f13c nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned short const*) + 432
25  libxpcom_core.dylib 	0x2c007a20 NS_ShutdownXPCOM_P + 164
26  org.mozilla.firefox 	0x0000e150 ScopedXPCOMStartup::~ScopedXPCOMStartup [unified]() + 60
27  org.mozilla.firefox 	0x00012660 XRE_main + 4280
28  org.mozilla.firefox 	0x0000d768 start + 432
29  org.mozilla.firefox 	0x0000d5e8 start + 48

Comment 5

[Zach Lipton [:zach]]

I've narrowed this down to fallout from bug 336978--we don't crash without that patch and we do crash with it. CC'ing some relevant folks from that bug. 

This isn't an Extension Manager bug anymore...Guessing Docshell?

Comment 6

[Robert Strong (they/them - no direct email)]

Zach, could you check whether this also affects Mac OS X using 1.5.0.5 since bug 336978 also landed on 1.8.0.5?

Comment 7

[Zach Lipton [:zach]]

'twas already on it Rob. Interestingly enough, this does not affect 1505, just 2.0. I haven't tried the trunk yet.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

So this is not Mac only?
Zach, could you test the first patch in bug 336978. Does that make
any difference comparing to the second patch which was checked in.

Comment 9

[Zach Lipton [:zach]]

Nope. We crash with the first patch as well. 

If you want me to try anything, ping me on irc (as 'zach').

Comment 10

[Marcia Knous [:marcia]]

I just tested this on the RC2 Windows build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060710 Firefox/2.0b1) and did not see the problem Zach is reporting on the Mac. I used Live HTTP headers as my test extension.

Comment 11

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

Can we get a confirmation on the Mac with a clean profile? Need to know how serious this is for blocking status.

Comment 12

[Zach Lipton [:zach]]

Marcia and Sheppy are both seeing this on Mac, and I have been using a clean profile. Sheppy has done some diagnosis and I understand a patch is coming real soon now. 

Comment 13

[Eric Shepherd [:sheppy]]

Attachment: fix for IsCallerChrome() to check for null sSecurityManager [checked in on 1.8branch]

This patch simply makes IsCallerChrome() check for a NULL sSecurityManager -- the routine gets called after shutting down the nsContentUtils object, and that causes the sSecurityManager to be deleted.  With the patch, it just returns PR_FALSE if the pointer is null, which is just what happens if an error occurs anyway.

Comment 14

[Benjamin Smedberg]

This is branch-only? The patch for bug 331117 should have prevented this on trunk.

Comment 15

[Eric Shepherd [:sheppy]]

I've not tried it on trunk.  That does sound like the fix for bug 331117 would prevent this problem too.

Comment 16

[Eric Shepherd [:sheppy]]

Sure enough, this crash doesn't occur on trunk.  I guess it comes down to whether we migrate the fix for bug 331117 to branch.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

According to TB there are crashes @nsContentUtils::IsCallerChrome also in 1.8.0 branch, even before the patch in bug 336978 went in.

Comment 18

[Zach Lipton [:zach]]

*** Bug 344268 has been marked as a duplicate of this bug. ***

Comment 19

[Benjamin Smedberg]

I originally wanted to, and dbaron was concerned about the ramifications; I'd be happy to prepare a branch version and ask for approval.

Comment 20

[Eric Shepherd [:sheppy]]

I think the patch I did isn't a bad idea to have anyway, regardless of whether the fix for bug 331117 gets migrated to the branch or not.  But one or both really ought to happen prior to release of Fx 2.

Comment 21

[Phil Ringnalda (:philor)]

*** Bug 344404 has been marked as a duplicate of this bug. ***

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 228834 [details] [diff] [review]
fix for IsCallerChrome() to check for null sSecurityManager [checked in on 1.8branch]

r+sr=jst for this change for the branch if we chose to take this as a spot-fix for this problem on the branch.

Comment 23

[Martijn Wargers (dead)]

Ok, bug 331117 now got fixed on the 1.8.1 branch, is the "fix for IsCallerChrome() to check for null sSecurityManager" patch still necessary for branch?

Comment 24

[Benjamin Smedberg]

I'd like QA to verify that crashes are no longer reproducable, but I'd say this is no longer needed.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

But the null check should be in 1.8.0 branch.

Comment 26

[Benjamin Smedberg]

Is this crash happening on the 1.8.0 branch?

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #26)
> Is this crash happening on the 1.8.0 branch?
> 

Apparently not this one, but see #17

Comment 28

[Zach Lipton [:zach]]

Tested against 1.8 branch build 2006071803 on Mac PPC. The crashes do seem to be fixed. 

That being said, talkback data does indicate that we are crashing in IsCallerChrome() with a different stack, and adding the null check seems relatively harmless, so it seems worth landing this on the branches anyway. 

Comment 29

[Benjamin Smedberg]

Comment on attachment 228834 [details] [diff] [review]
fix for IsCallerChrome() to check for null sSecurityManager [checked in on 1.8branch]

I've committed this additional safety patch on the 1.8 branch.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should this be marked fixed1.8.1?

Comment 31

[Adam Guthrie]

I corrected the verified1.8 keyword to be verified1.8.1

Comment 32

[Zach Lipton [:zach]]

My mistake. This should be marked verified1.8.1. It has not yet landed on 1.8.0.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 228834 [details] [diff] [review]
fix for IsCallerChrome() to check for null sSecurityManager [checked in on 1.8branch]

approved for 1.8.0 branch, a=dveditz for drivers

Comment 34

[Benjamin Smedberg]

Fixed on MOZILLA_1_8_0_BRANCH. Marking fixed because I don't see this on trunk (bug 331117 should have fixed all the occurences I know of).