497934 - fennec crashes [@ nsFocusManager::GetCommonAncestor] whenever I get focus on a text field

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

for the last two days I have been getting .cab files from: ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mobile-trunk/

They install fine, and fennec launches fine upon clicking the icon.  The problem is when I tap in the URL bar Fennec crashes.  Same with bookmarks->manage->new folder as well as a new tab.  

This is repeatable and happens every time.  I am on a HTC Touch Pro which I have used for all my previous testing successfully.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix

I do not know exactly reason but dsti->GetParent(getter_AddRefs(dsti)) causes crash on WinCE.

This patch fixes startup crash and the crash when focusing on URL entry.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch

Rewrite all dsti->GetParent(getter_AddRefs(dsti)) in nsFocusManager.cpp.

Comment 3

[Masatoshi Kimura [:emk]]

This bug was not Fennec Specific.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 383070 [details] [diff] [review]
Revised patch

Asking Neil for a review

Comment 6

[Neil Deakin (away until Sept 6)]

Comment on attachment 383070 [details] [diff] [review]
Revised patch

do_GetInterface and do_QueryInterface already check for null arguments, so there isn't a need to check beforehand. There's a few places where these could be removed.

>     nsCOMPtr<nsIDocShellTreeItem> dsti = do_QueryInterface(webnav);
>-    dsti->GetParent(getter_AddRefs(dsti));
>+    nsCOMPtr<nsIDocShellTreeItem> parentDsti;
>+    if (!dsti) 
>+      return;

Move this check before the parentDsti declaration.


>       if (IsWindowVisible(window) != isVisible)
>-        break;
>+        return;

I'd rather keep the break than have an early return.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

dsti->GetParent(getter_AddRefs(dsti)) is bad, and is undefined in C++ according to bz.  getter_AddRefs could be evaluated before the operator ->, resulting in a potentially bogus call.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Update patch

Address Neil's comment.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Does this need an sr?

Comment 10

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

Comment on attachment 383374 [details] [diff] [review]
Update patch

>   while (dsti) {
>     if (dsti == ancestordsti)
>       return PR_TRUE;
>-    dsti->GetParent(getter_AddRefs(dsti));
>+    nsCOMPtr<nsIDocShellTreeItem> parentDsti;
>+    dsti->GetParent(getter_AddRefs(parentDsti));
>+    dsti = parentDsti;
dsti.swap(parentDsti);

>   do {
>     parents1.AppendElement(dsti1);
>-    dsti1->GetParent(getter_AddRefs(dsti1));
>+    nsCOMPtr<nsIDocShellTreeItem> parentDsti1;
>+    dsti1->GetParent(getter_AddRefs(parentDsti1));
>+    dsti1 = parentDsti1;
dsti1.swap(parentDsti1);


>   } while (dsti1);
>   do {
>     parents2.AppendElement(dsti2);
>-    dsti2->GetParent(getter_AddRefs(dsti2));
>+    nsCOMPtr<nsIDocShellTreeItem> parentDsti2;
>+    dsti2->GetParent(getter_AddRefs(parentDsti2));
>+    dsti2 = parentDsti2;
dsti2.swap(parentDsti2);

Comment 11

[Neil Deakin (away until Sept 6)]

Do you need someone to check this in?

Comment 12

[Hiroyuki Ikezoe (:hiro)]

yes please.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in (with swap fix).

http://hg.mozilla.org/mozilla-central/rev/2c3e19e8ac84

Comment 14

[Doug Turner (:dougt)]

could this have caused the regression bug 500275?

Comment 15

[Doug Turner (:dougt)]

i retract comment #14.  The crash probably masked the problem we are having with the software keyboard.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)