This patch is very similar to the one in bug 482928. In bug 482928, Masayuki changed the reference from (in Mac terms) NSView* to NSWindow*, because windows live longer than views. However, in bug 420491 I need to create a new NSWindow when entering full screen mode, so holding a pointer to an NSWindow isn't doing much good, either. In this patch, I'll change mWidget to store an nsIWidget and query for the native widget only when we need it.

Unit tests passed on the tryserver build, apart from known intermittent failures.

Comment on attachment 381025 [details] [diff] [review] patch v1: store the nsIWidget* instead > nsThebesDeviceContext::FindScreen(nsIScreen** outScreen) > { > - if (mWidget) > - mScreenManager->ScreenForNativeWidget(mWidget, outScreen); > + if (mWidget && mWidget->GetNativeData(NS_NATIVE_WINDOW)) > + mScreenManager->ScreenForNativeWidget(mWidget->GetNativeData(NS_NATIVE_WINDOW), > + outScreen); @@ -2305,26 +2305,17 @@ DocumentViewerImpl::CreateDeviceContext( > - nsNativeWidget nativeWidget = nsnull; > - if (aWidget) { > - // The device context should store NS_NATIVE_WINDOW of TopLevelWidget, > - // because NS_NATIVE_WIDGET and NS_NATIVE_WINDOW of the given widget can be > - // destroyed earlier than the device context. > - nsIWidget* toplevelWidget = aWidget->GetTopLevelWidget(); > - NS_ASSERTION(toplevelWidget, "GetTopLevelWidget returns NULL"); > - nativeWidget = aWidget->GetNativeData(NS_NATIVE_WINDOW); > - } > - mDeviceContext->Init(nativeWidget); > + mDeviceContext->Init(aWidget); Looks like you changes the stored widget from top level widget to the given widget. Is this right? I changed to use top level widget's native widget in bug 482928, but you cancels it. Doesn't this patch reopen the bug 482928?

Oh, good catch! I totally missed the top level thing. Let me test.

So, looking at the code it don't think that this will regress bug 482928, but rather bug 479749. The crash in bug 479749 occured because something tries to access a NSView that has been destroyed. But afaik NSViews are only destroyed when their container nsIWidget is destroyed, so my patch will hit exactly the same issue. However, I haven't been able to reproduce bug 479749 yet.

(In reply to comment #4) > So, looking at the code it don't think er, "I don't think"

Comment on attachment 381143 [details] [diff] [review] v2: use the top level widget looks ok for me. > I don't think that this will regress bug 482928, but > rather bug 479749. er, right.

There's one other caller of nsThebesDeviceContext::Init, in gfx/tests/coverage/nsCoverage.cpp. I'll fix that on checkin.

Comment on attachment 381143 [details] [diff] [review] v2: use the top level widget sorry for the delay

As Markus asked me on IRC we should test it with the Mozmill test for entering/leaving private browsing. I'll check it tomorrow.

Markus, the Mozmill test works fine. Looks like we haven't regressed anything.