Closed Bug 495920 Opened 14 years ago Closed 14 years ago

nsThebesDeviceContext shouldn't have a reference to a native widget

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #381025 - Flags: superreview?(roc)
Attachment #381025 - Flags: review?(smichaud)
Attachment #381025 - Flags: review?(masayuki)
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.
Attachment #381025 - Attachment is obsolete: true
Attachment #381143 - Flags: review?(masayuki)
Attachment #381025 - Flags: superreview?(roc)
Attachment #381025 - Flags: review?(smichaud)
Attachment #381025 - Flags: review?(masayuki)
(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.
Attachment #381143 - Flags: review?(masayuki) → review+
Attachment #381143 - Flags: superreview?(vladimir)
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
Attachment #381143 - Flags: superreview?(vladimir) → superreview+
http://hg.mozilla.org/mozilla-central/rev/0e97145d2cce
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
As Markus asked me on IRC we should test it with the Mozmill test for entering/leaving private browsing. I'll check it tomorrow.
Depends on: 498230
Markus, the Mozmill test works fine. Looks like we haven't regressed anything.
You need to log in before you can comment on or make changes to this bug.