627938 - Bad call to nsGlobalChromeWindow::CleanUp causes segfault

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Disconnect destroyed nsGlobalChromeWindow.

CleanUp is called from the nsGlobalWindow destructor, after the more-derived nsGlobalChromeWindow destructor has been called, but the mMessageManager->Disconnect() used from the nsGlobalWindow destructor is part of the nsGlobalChromeWindow.

1120	  if (mIsChrome && static_cast<nsGlobalChromeWindow*>(this)->mMessageManager) {
1121	    static_cast<nsFrameMessageManager*>(
1122	       static_cast<nsGlobalChromeWindow*>(
1123	         this)->mMessageManager.get())->Disconnect();
1124	  }

As mrbkap points out, the CleanUp code is called from multiple places, so there should be a flag passed to indicate whether the nsGlobalChromeWindow bits have already been destroyed.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Fix chrome window destructor.

No longer segfaults on my box.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 506027 [details] [diff] [review]
Fix chrome window destructor.

I think jst should actually review this.

Comment 3

[Johnny Stenback (:jst)]

I wonder if calling CleanUp() in the nsGlobalChromeWindow dtor wouldn't be a more correct fix here, calling it from there would mean that the call in the nsGlobalWindow dtor would return early, before we hit the bad cast. Smaug, any thoughts? Chris, does that fix this issue for you (assuming you can reproduce)?

I'd be happy to take the attached fix for now if we deem anything else has too many non-obvious side effects, but it *seems* fine to me to call CleanUp() from the nsGlobalChromeWindow dtor.

Comment 4

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

Hmm, setting mIsChrome to false is error prone, I think.
Though, so is the whole CleanUp handling.

Could you just add a boolean flag to nsGlobalWindow to indicate if
message manager needs to be disconnected. Then set it true when
messageManager is created, and false when disconnected.
And also disconnect in chromewindow dtor.

A bit ugly, but should be quite safe. And that is what we need at this point, 
IMO.


And yes, this all is my fault :(

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Clean manager flag.

I can confirm that this avoids the crash when I get back to my machine in the office, but seeing as how the other fix worked, I'm pretty sure we have the issue nailed down.

Comment 6

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

Comment on attachment 506157 [details] [diff] [review]
Clean manager flag.

Btw, after FF4 we should probably change GlobalWindow deletion to be closer to
what happens with DOM Nodes - dtor is in general rather dummy, but
most of the clean up happens in LastRelease.
There is a reason why we have
NS_IMPL_CYCLE_COLLECTING_RELEASE_AMBIGUOUS_WITH_DESTROY macro.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 506157 [details] [diff] [review]
Clean manager flag.

Not knowing how and when this is triggered I can't say I think this blocks, but I'd love to see this patch land for 2.0 either way.

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #7)
> Not knowing how and when this is triggered I can't say I think this blocks, but
> I'd love to see this patch land for 2.0 either way.

Not sure why this was burning tip mochitests on my box but nobody else's... destructor order should be deterministic and destructing the subclass parts will always create invalid-memory-pattern on debug builds? I must be missing something.

Pushing to try, just to make sure it comes up as green as the last patch!

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

I forgot I had already pushed that patch to try and it went green -- here's the checkin: http://hg.mozilla.org/mozilla-central/rev/c16db7731c12