Last Comment Bug 778355 - nsWebShellWindow destructor prevents nsXULWindow destructor from clearing pointers from tree owners
: nsWebShellWindow destructor prevents nsXULWindow destructor from clearing poi...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Justin Lebar (not reading bugmail)
:
:
Mentors:
Depends on: 775676
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-27 16:43 PDT by neil@parkwaycc.co.uk
Modified: 2012-08-02 19:09 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch (707 bytes, patch)
2012-07-27 16:51 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Patch, v1 (935 bytes, patch)
2012-07-27 17:09 PDT, Justin Lebar (not reading bugmail)
roc: review+
Details | Diff | Splinter Review
Part 1a, v1: Release the references after we release the lock. (1.90 KB, patch)
2012-08-01 10:38 PDT, Justin Lebar (not reading bugmail)
roc: review+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2012-07-27 16:43:40 PDT
For some reason, nsWebShellWindow's destructor clears out the nsIWidget member inherited from nsXULWindow, accidentally bypassing nsXULWindow's destruction code. Previously someone has always managed to invoke Destroy manually, however as of bug 777063 it's definitely possible to tickle the code so that nsWebShellWindow gets destroyed before Destroy gets invoked. This prevents nsXULWindow from clearing the pointers in the tree owners which are now stale and promptly crash if any of the tree owner's methods get called.

As it happens, Destroy has an identical copy of the code to clear that member.
Comment 1 neil@parkwaycc.co.uk 2012-07-27 16:44:40 PDT
I meant bug 775676 of course...
Comment 2 neil@parkwaycc.co.uk 2012-07-27 16:51:54 PDT
Created attachment 646761 [details] [diff] [review]
Proposed patch
Comment 3 Justin Lebar (not reading bugmail) 2012-07-27 17:09:05 PDT
Created attachment 646769 [details] [diff] [review]
Patch, v1
Comment 4 Justin Lebar (not reading bugmail) 2012-07-30 09:23:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/20b67f48676c
Comment 5 Matt Brubeck (:mbrubeck) 2012-07-30 11:59:55 PDT
Sorry, I backed this out on inbound because it looks like it introduced a perma-orange in make check on Windows debug builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72966f49cf09

https://tbpl.mozilla.org/php/getParsedLog.php?id=13975366&tree=Mozilla-Inbound

mozilla::ShutdownXPCOM[xul +0x1253D82]
ScopedXPCOM::~ScopedXPCOM[TestAppShellSteadyState +0x37E0]
main[TestAppShellSteadyState +0x5970]
__tmainCRTStartup[TestAppShellSteadyState +0x97EF]
mainCRTStartup[TestAppShellSteadyState +0x961F]
BaseThreadInitThunk[kernel32 +0x133CA]
RtlInitializeExceptionChain[ntdll +0x39ED2]
RtlInitializeExceptionChain[ntdll +0x39EA5]

###!!! Deadlock may happen NOW!

###!!! ASSERTION: Potential deadlock detected:
Cyclical dependency starts at
Mutex : nsWindowMediator.mListLock (currently acquired)
Cycle completed at
Mutex : nsWindowMediator.mListLock (currently acquired)
Comment 6 neil@parkwaycc.co.uk 2012-07-30 12:19:47 PDT
Annoying. The test shuts down XPCOM while the window is still open, causing the window mediator to unregister all of its windows. Because it's the owner of last resort, this causes the destructor to run, which (with this patch) causes the nsXULWindow to try to unregister itself again.
Comment 7 Justin Lebar (not reading bugmail) 2012-07-31 20:18:09 PDT
Sounds like we should make sure we don't re-enter nsXULWindow::Destroy.
Comment 8 Justin Lebar (not reading bugmail) 2012-07-31 20:28:52 PDT
https://tbpl.mozilla.org/?tree=Try&rev=3cd2a329942b
Comment 9 neil@parkwaycc.co.uk 2012-08-01 00:40:19 PDT
(In reply to Justin Lebar from comment #7)
> Sounds like we should make sure we don't re-enter nsXULWindow::Destroy.
We don't, this is the only call to nsXULWindow::Destroy in the test.
Comment 10 Justin Lebar (not reading bugmail) 2012-08-01 07:57:10 PDT
Okay, let's try this.  https://tbpl.mozilla.org/?tree=Try&rev=120d646c3cf3
Comment 11 Justin Lebar (not reading bugmail) 2012-08-01 10:38:50 PDT
Created attachment 648005 [details] [diff] [review]
Part 1a, v1: Release the references after we release the lock.

As Neil pointed out on IRC, we don't actually need this additional block scope, so long as the array is declared before the auto-lock.  I think it clarifies things, but I'm not attached to it.
Comment 12 Justin Lebar (not reading bugmail) 2012-08-02 07:39:29 PDT
Comment on attachment 648005 [details] [diff] [review]
Part 1a, v1: Release the references after we release the lock.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3111cd3bc5f5
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-08-02 19:09:21 PDT
https://hg.mozilla.org/mozilla-central/rev/3111cd3bc5f5

Note You need to log in before you can comment on or make changes to this bug.