Closed Bug 497934 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: User events and focus handling, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed
fennec 1.0a2-wm+ ---

People

(Reporter: jmaher, Assigned: hiro)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Fix (obsolete) — Splinter Review
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.
Attached patch Revised patch (obsolete) — Splinter Review
Rewrite all dsti->GetParent(getter_AddRefs(dsti)) in nsFocusManager.cpp.
Assignee: nobody → ikezoe
Attachment #383065 - Attachment is obsolete: true
This bug was not Fennec Specific.
Assignee: ikezoe → nobody
Component: Windows Mobile → Event Handling
Product: Fennec → Core
QA Contact: mobile-windows → events
Duplicate of this bug: 498250
OS: Windows Mobile 6 Professional → All
Hardware: ARM → All
Comment on attachment 383070 [details] [diff] [review]
Revised patch

Asking Neil for a review
Attachment #383070 - Flags: review?(enndeakin)
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.
Blocks: 178324
Summary: fennec crashes whenever I get focus on a text field → fennec crashes [@ nsFocusManager::GetCommonAncestor] whenever I get focus on a text field
tracking-fennec: --- → 1.0a2-wm+
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.
Flags: blocking1.9.2+
Priority: -- → P1
Attached patch Update patchSplinter Review
Address Neil's comment.
Assignee: nobody → ikezoe
Attachment #383070 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #383374 - Flags: review?(enndeakin)
Attachment #383070 - Flags: review?(enndeakin)
Attachment #383374 - Flags: review?(enndeakin) → review+
Does this need an sr?
Attachment #383374 - Flags: superreview+
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);
Do you need someone to check this in?
Keywords: checkin-needed
yes please.
Checked in (with swap fix).

http://hg.mozilla.org/mozilla-central/rev/2c3e19e8ac84
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
could this have caused the regression bug 500275?
i retract comment #14.  The crash probably masked the problem we are having with the software keyboard.
Duplicate of this bug: 499796
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)
Keywords: fixed1.9.2
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.