Crash [@ nsFocusManager::GetCommonAncestor], part 2

RESOLVED FIXED

Status

()

Core
XUL
--
critical
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Neil Deakin)

Tracking

({crash, regression, testcase})

Trunk
x86
Mac OS X
crash, regression, testcase
Points:
---
Bug Flags:
wanted1.9.2 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ss:b2], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

9 years ago
Created attachment 388602 [details]
testcase

See testcase, which crashes current trunk build after 600ms.

The iframe content consists of this:
<html><head></head>
<body onunload="window.frameElement.parentNode.removeChild(window.frameElement)" tabindex="1">
<script>
document.body.focus();
</script>
</body>
</html>

http://crash-stats.mozilla.com/report/index/c6767d12-cd84-427c-8e62-62fc92090714?p=1
0  	xul.dll  	nsFocusManager::GetCommonAncestor  	 dom/base/nsFocusManager.cpp:1135
1 	xul.dll 	nsFocusManager::SetFocusInner 	dom/base/nsFocusManager.cpp:1056
(Assignee)

Comment 1

9 years ago
This is caused because nsFocusManager::mFocusedWindow isn't being reset when it's a child or descendant of the <iframe> being removed. The solution is to reset the focused window in this case. I am working on a patch.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Flags: blocking1.9.2?
Whiteboard: [ss:b2]
(Assignee)

Comment 2

9 years ago
Created attachment 388972 [details] [diff] [review]
fix

also added some assertions in GetCommonAncestor to prevent any future crashes.
Attachment #388972 - Flags: review?(Olli.Pettay)

Comment 3

9 years ago
Comment on attachment 388972 [details] [diff] [review]
fix

130,21 @@ nsFocusManager::GetCommonAncestor(nsPIDO
>                                   nsPIDOMWindow* aWindow2)
> {
>   nsCOMPtr<nsIWebNavigation> webnav(do_GetInterface(aWindow1));
>   nsCOMPtr<nsIDocShellTreeItem> dsti1 = do_QueryInterface(webnav);
> 
>   webnav = do_GetInterface(aWindow2);
>   nsCOMPtr<nsIDocShellTreeItem> dsti2 = do_QueryInterface(webnav);
> 
>+  NS_ASSERTION(dsti1, "possible ancestor has no nsIDocShellTreeItem");
>+  NS_ASSERTION(dsti2, "possible descendant has no nsIDocShellTreeItem");
>+  if (!dsti1 || !dsti2)
>+    return nsnull;
>+
I don't usually like this kind of use of assertions, when assertion doesn't
mean anything fatal.
Maybe you could just add NS_ENSURE_TRUE(dsti1 && dsti2, PR_TRUE);
Attachment #388972 - Flags: superreview+
Attachment #388972 - Flags: review?(Olli.Pettay)
Attachment #388972 - Flags: review+
(Assignee)

Comment 4

9 years ago
(In reply to comment #3)

> I don't usually like this kind of use of assertions, when assertion doesn't
> mean anything fatal.
> Maybe you could just add NS_ENSURE_TRUE(dsti1 && dsti2, PR_TRUE);

If dsti1 has no docshell, it means we're trying to focus a bad window, which shouldn't happen. If dsti2 has no docshell, it means the existing focus in a window that isn't valid, which should already have been dealt with elsewhere. Either way, it's a bad state.

I can use NS_ENSURE_TRUE, but I'll use two separate lines, to make it clearer which one failed.
(Assignee)

Comment 5

8 years ago
http://hg.mozilla.org/mozilla-central/rev/d15d5d9abfd1
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Flags: blocking1.9.2? → wanted1.9.2+
(Assignee)

Updated

8 years ago
Flags: in-testsuite+
Crash Signature: [@ nsFocusManager::GetCommonAncestor]
You need to log in before you can comment on or make changes to this bug.