530070 - Firefox crashes in [@ nsWindow::GetParentWindow(int)] called from nsLayoutUtils::TranslateWidgetToView

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

http://crash-stats.mozilla.com/report/index/d8af48a5-3300-421e-ba3f-320b82091120

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Just the stack for reference:

0  	xul.dll  	nsWindow::GetParentWindow  	 widget/src/windows/nsWindow.cpp:1063
1 	xul.dll 	nsWindow::GetParent 	widget/src/windows/nsWindow.cpp:1025
2 	xul.dll 	GetWidgetOffset 	layout/base/nsLayoutUtils.cpp:839
3 	xul.dll 	nsLayoutUtils::TranslateWidgetToView 	layout/base/nsLayoutUtils.cpp:860
4 	xul.dll 	nsLayoutUtils::GetEventCoordinatesRelativeTo 	layout/base/nsLayoutUtils.cpp:680
5 	xul.dll 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:6127

I think we should close this bug for now until we know if it's not exploidable.

Comment 2

[Daniel Veditz [:dveditz]]

should we remove the brackets from nsLayoutUtils::TranslateWidgetToView in the summary? If we ever get a crash at that address we'll be fooling crash-stats into linking here.

If the line number in the stack is correct it seems like we got a bogus (already destroyed?) widget out of the HWND. At a brief glance it doesn't look like we're doing anything all that dangerous with it, just trying to read its size and position. We're not writing to it, writing to a location returned by it, or calling any virtual functions.

Comment 3

[Daniel Veditz [:dveditz]]

I take that back, GetBounds() is virtual.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #2)
> should we remove the brackets from nsLayoutUtils::TranslateWidgetToView in the
> summary? If we ever get a crash at that address we'll be fooling crash-stats
> into linking here.

Using the | character to separate the different frames is the way we normally go, right? If not feel free to update the summary again.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Bug 506108 comment 7 answers that. Reverting Summary.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Mats, can you look into this? No steps to reproduce, so may not be fixable.

Comment 7

[Mats Palmgren (inactive)]

Attachment: wip

I found one problem, probably not the crash cause though.
In OnDestroy we set mOnDestroyCalled to PR_TRUE (which means GetParent()
will return nsnull) before calling  nsBaseWidget::Destroy() which
prevents it from removing us from the parent's child list.
(STR: drag a tab onto the desktop (creating a new window for it) --
each time the child list grows by one)).

The child window in question has mWnd=NULL and mOnDestroyCalled=PR_TRUE
so I don't think it can cause any trouble (other than the temporary
"leak").

Comment 8

[Mats Palmgren (inactive)]

The nsWindow parent relationship looks a bit fragile (storing the native
parent HWND on a property on the child HWND), it's a "weak ref" and we
depend on Windows to remove these properties in a timely fashion
(can an event be dispatched to a child window after/during a WM_DESTROY
on a parent?).

Since we can't find the error, I suggest we add protective code to avoid
crashing.  I would re-instate the "unnecessary" SetParent(nsnull) in
OnDestroy as a first step.
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#6039
Perhaps also do SetParent(nsnull) on all children in nsBaseWidget::Destroy().

If the crashing continues I suggest we give up the current design and
use a strong ref mParent (as we do on gtk2).

Comment 9

[Jim Mathies [:jimm]]

(In reply to comment #8)
> If the crashing continues I suggest we give up the current design and
> use a strong ref mParent (as we do on gtk2).

^^ that would be my preference as well, but I found this wasn't simple to implement. I didn't spend a lot of time on it though. If we have the time now, I'd say taking a shot at remove the prop is the best bet to fixing all these issues.

Comment 10

[Mats Palmgren (inactive)]

Another option would be to add the SetParent(nsnull) per above +
NS_ADDREF/RELEASE the parent in SetParent.  That would probably
fix the crash but we get a leak instead?

Comment 11

[Mats Palmgren (inactive)]

Attachment: wip10

Here's a mParent solution for trunk... I think I can cook this down to a small
low-risk fix for branches.

Comment 12

[Mats Palmgren (inactive)]

Attachment: Low-risk patch for 1.9.2

Keep a strong ref on the parent window, but don't use it for anything.

Comment 13

[Jim Mathies [:jimm]]

+    parent = aParent ? (HWND)aParent->GetNativeData(NS_NATIVE_WINDOW) : NULL;

nit - let's stick with nsnull here.

We're going to land this on m-c first for a bit right?

Comment 14

[Mats Palmgren (inactive)]

(In reply to comment #13)
> +    parent = aParent ? (HWND)aParent->GetNativeData(NS_NATIVE_WINDOW) : NULL;
> 
> nit - let's stick with nsnull here.

I'm sticking with the established convention here - to use NULL
for Windows types such as HWND, HCURSOR, HHOOK, HBRUSH etc
and nsnull for Mozilla types.

> We're going to land this on m-c first for a bit right?

I've landed the 1.9.2 patch on m-c for baking:
http://hg.mozilla.org/mozilla-central/rev/1fb768ca019e

Leaving the bug open to revert that and land the correct trunk patch later.

Comment 15

[Mats Palmgren (inactive)]

Backed out changeset 1fb768ca019e due to unexpected leak:
1   nsBaseDragService
521 nsNativeDragTarget

Comment 16

[Mats Palmgren (inactive)]

Attachment: Low-risk patch for 1.9.2, v2

It appears there are circular strong references between
nsWindow/nsNativeDragTarget/nsDragService.
I haven't figured out exactly why it leaked but calling
'mDragService->EndDragSession(PR_FALSE)' when the DnD is canceled
and doing the widget Destroy() call in nsXULWindow::Destroy()
fixes it.  This patch is slightly more risky than the last,
but still low-risk I think.

TryServer unit tests pass without leaking.
I also tested the TryServer builds manually on all 3 platforms,
doing nasty stuff like detaching tabs with a plugin in them and such.

Comment 17

[Jim Mathies [:jimm]]

+  else {
+    // Tell the drag service we're done so it can release objects.
+    mDragService->EndDragSession(PR_FALSE);
+  }

This feels like a bit of a hack. Is there any assurance DragOver will always get called after the drag operation canceled? What about unit tests?

Comment 18

[Jesse Ruderman]

> Leaving the bug open to revert that and land the correct trunk patch later.

Please mark this bug as fixed when you reland the patch on trunk.  Leaving bugs open for followups makes it hard to track what security holes affect a given branch, and makes it hard to track regressions.  The new bug can be security-sensitive but [sg:nse].

Comment 19

[Jim Mathies [:jimm]]

(In reply to comment #17)
> +  else {
> +    // Tell the drag service we're done so it can release objects.
> +    mDragService->EndDragSession(PR_FALSE);
> +  }
> 
> This feels like a bit of a hack. Is there any assurance DragOver will always
> get called after the drag operation canceled? What about unit tests?

Mats, is it ok if I take this bug over? Id like to dig into this a bit and understand why this is needed.

Comment 20

[Mats Palmgren (inactive)]

Attachment: Low-risk patch for 1.9.2, v3

> Is there any assurance DragOver will always
> get called after the drag operation canceled?

Not that I can see, no.
So, here's a version that doesn't depend on reaching DragOver.
I'm replacing the mDragCancelled flag with a DragCancel() call
that does the cleanup directly.

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 446003 [details] [diff] [review]
Low-risk patch for 1.9.2, v3

thanks for the update!

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What has to be done for trunk here? The mixture of trunk and branch comments is a bit confusing.

Comment 23

[Mats Palmgren (inactive)]

I landed the 1.9.2 patch on trunk for baking, but had to back out due
to unexpected Tinderbox orange on Linux.  The previously intermittent orange
bug 527099 and bug 551538 (Mooth), and bug 542580 (Mo5) became perma-orange.
I've debugged it I understand why it occurs with the patch -- adding the
nsWindow::Destroy call in nsXULWindow::Destroy makes SetPrompt(PR_FALSE),
which comes later, fail which in turn messes up window focus/activation.
I'm going to file a separate bug on this, hoping it'll fix those
"unable to restore focus" errors that is causing intermittent orange
on Linux.  I don't know yet if that fix will be safe for branch though.

I've pushed the patch without the explicit Destroy to Try to see if it
still leaks (we've changed mDragCancelled since then which might have helped).  I'll report back with the results...

Comment 24

[Mats Palmgren (inactive)]

I mean SetModal(PR_FALSE) of course, not SetPrompt.
BTW, that call is in nsXULWindow::ShowModal() after exiting the modal loop.

Comment 25

[Mats Palmgren (inactive)]

Yes, it still leaks nsNativeDragTarget without the explicit 
mWindow->Destroy().  :-(
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1274669543.1274670507.10879.gz

Jim, feel free to figure out if there's a better way to plug that leak.
I'm going to see if I can fix the focus problem on Linux (bug 567704)
in a safe way so we can keep the Destroy() call.

Comment 26

[Mats Palmgren (inactive)]

There's a fix in bug 567704 for the focus problem, it passed TryServer
unit tests together with this patch.

Comment 27

[Karl Tomlinson (:karlt)]

Is 1.9.1 affected by the same issue?

Comment 28

[Mats Palmgren (inactive)]

All branches as far as I can tell from crash-stats.

Comment 29

[Mats Palmgren (inactive)]

Landed the lower-risk fix on trunk, will ask for branch approvals
(together with bug 567704) after baking it for a while.
http://hg.mozilla.org/mozilla-central/rev/9a3a1fa8233a

I filed bug 569583 for the additional (riskier) trunk changes (in wip10).

Comment 30

[Dietrich Ayala (:dietrich)]

Looks like this might have caused blocking regression bug 585287.