52940 - Crash on exit

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P1)

Description

[Bob Kanefsky]

I had been successfully testing a local file copy of
  http://www.roving-mouse.com/planetary/sizes.html
and had given a Quit command.  It closed the browser window,
then it crashed before the windows behind it could refresh
their newly exposed regions.

I believe I had only been working with that page, and only
in the original browser window.

Comment 1

[Bob Kanefsky]

Attachment: MacsBug StdLog

Comment 2

[Bob Kanefsky]

Attachment: Local file I was working with (see above for where to find inline images and scripts)

Comment 3

[Doron Rosenberg (IBM)]

were you using composer?

Comment 4

[Bob Kanefsky]

No, I wasn't using Composer.  I was editing the file in another application and 

testing it in Mozilla's browser.

Comment 5

[Asa Dotzler [:asa]]

I also saw this but only on the first quit with 091804 mozilla build on OS9.
Over to XPConnect.

Comment 6

[John Bandhauer]

I'm on it. This looks like it is going to be the same as bug 50611.

I've been trying to reproduce and debug this.

I've got a theory that the use of a DOM context for the safe context is screwing 
us at shutdown. I'm following up that idea.

Comment 7

[John Bandhauer]

Yup. That's the problem. The hidden window cx is used in call to 
SetSafeJSContext and that cx gets destroyed before xpconnect gets shutdown. 
I'll look into a fix. cc'ing brendan and mccabe because they perpetrated this 
stuff while I was away. But I don't mind fixing it.

Comment 8

[John Bandhauer]

Attachment: proposed fix

Comment 9

[John Bandhauer]

Attached a proposed fix. Looking for reviewers and approvers.

Comment 10

[John Bandhauer]

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

Comment 11

[Brendan Eich [:brendan]]

Damn, mccabe and I blew it -- we ass-u-med a certain shutdown order.  Your patch
looks good to me, a=[e-mail] (FWIW :-/).

/be

Comment 12

[Mike McCabe]

ClearXPConnectSafeContext can return success even if it doesn't clear the
thread's safe context.  Should it return another result if cx != safe_cx? When
would this occur?

Comment 13

[John Bandhauer]

Welcome back Mike.

Note that this code ignores the result anyway.

It would occur if someone else had set the safe context since we set it to the 
hidden window. We really just want to know that the safe context is no longer 
set to our context that we arebout to destroy.

fwiw, I wish you guys had added this method to nsIThreadJSContextStack (i.e. 
made 'attribute JSContext safeJSContext' not be readonly) rather than 
adding it to nsIXPConnect. I guess I had months to notice and do something. Oh 
well.

Comment 14

[John Bandhauer]

fix checked in to trunk.

Comment 15

[Mike McCabe]

You're right, a setter for safeJSContext would have been better, and would have
eliminated the yucky safe-context-for-current-thread semantics of
setSafeJSContextForCurrentThread.  Sorry!  And thanks for the post-facto design
tip.

Comment 16

[Bob Kanefsky]

I've just tried 2000091920 and didn't see the problem.  I would mark this as 

Verified if I could reproduce the original problem in 2000091509, but I can't 

make it crash now.  It must depend on the exact timing and what the threads are 

doing.  (I haven't looked at the patch, just the discussion.)  Asa, do you have a 

reliable way to reproduce it in the old build?  If no one can reproduce it 

anywhere, I'd say we should assume it's fixed.

Comment 17

[Bob Kanefsky]

Since it hasn't happened again to me and no one else spoke up,
there's no reason to doubt that the fix worked.  Marking Verified.