Closed Bug 306067 Opened 19 years ago Closed 19 years ago

null pointer dereference crash [@ nsAutoCompleteController::HandleKeyNavigation]

Categories

(Toolkit :: Autocomplete, defect)

Product:
Toolkit
Component:
Autocomplete
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: dbaron, Assigned: bryner)

References

Details

(Keywords: crash, fixed1.8.1, verified1.8.0.1)

Crash Data

Attachments

(4 files)

gdb session showing JS stack and C++ stack
30.64 KB, text/plain; charset=utf-8
Details
quick fix
1.29 KB, patch
dbaron
: first-review+
dbaron
: second-review+
dveditz
: approval1.8.0.1+
dveditz
: approval1.8.1+
Details | Diff | Splinter Review
stack of mInput being nulled
11.91 KB, text/plain
Details
patch (against 1.5 branch, but should be portable to trunk)
18.00 KB, patch
roc
: first-review+
benjamin
: approval1.8.1-
Details | Diff | Splinter Review 
I just ran into a 100%-reproducable null pointer dereference crash in
nsAutoCompleteController::HandleKeyNavigation .

Steps to reproduce:
 1. get lots of URLs beginning with "tinderbox.mozilla.org" in your autocomplete
history (or use something else)
 2. Press Ctrl-L to focus URL bar
 3. Type "tind"
 4. Hold down the "PgDn" key

Actual results:  Crash when you get to the bottom of the autocomplete results list.

Debugging session with C++ and JS stack information to be attached.

[26 09:42:38] <bryner> dbaron: i think we should probably just add the obvious
null check in nsAutoCompleteController
[26 09:43:15] <dbaron> 397          
mInput->GetCompleteSelectedIndex(&completeSelection);
[26 09:43:15] <dbaron> (gdb) p mInput
[26 09:43:15] <dbaron> $37 = {mRawPtr = 0x0}
...
[26 09:45:47] <bryner> dbaron: the current assumption is that a focus event will
occur and set up mInput prior to and keypress events... which isn't really all
that unreasonable
This is in a Linux debug+optimized build, checked out around 8AM PDT on Thursday
2005-08-25.
Line 330 is the third line of:

      <method name="onKeyPress">
        <parameter name="aEvent"/>
        <body><![CDATA[

line 428 is the first line of:

      <handler event="keypress" phase="capturing"
               action="return this.onKeyPress(event);"/>
Severity: normal → critical
Keywords: crash
Flags: blocking1.8b4?
Flags: blocking1.8b4? → blocking1.8b4+
*** Bug 306093 has been marked as a duplicate of this bug. ***
OS: Linux → All
Version: unspecified → Trunk
Jay - where does this land on the talkback radar?
Keywords: qawanted
This is the #11 topcrash for 1.5b1
(http://talkback-public.mozilla.org/reports/firefox/FF15b1/FF15b1-topcrashers.html)
and there are quite a few "nsAutoComplete" incidents with this stack signature
in current Talkback data:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=nsAutoComplete&vendor=All&product=All&platform=All&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid

Since we know what is causing this and a simple null check will most likely fix
it, I think we should get this fixed asap so we can verify the results before
beta 2.
bryner or dbaron can you hook us up with a patch (or Jay)?
Assigning to bryner to see if he can put together a quick patch.
Assignee: nobody → bryner
Since we've already accessed mInput if we get to this line, it must be the 
case that calling popup->SelectBy() can null it out.  I'll make a patch to 
fetch completeSelection before we do that, which should put us back to where 
we were before the patch for bug 270697.
Attached patch quick fix — — Splinter Review 
This should eliminate any possibility of crashing at the location given in
these talkback reports.
Attachment #197485 - Flags: second-review?(dbaron)
Attachment #197485 - Flags: first-review?(dbaron)
What about all the other dereferences of mInput later, if completeSelection is
true?  (It's dereferenced in both codepaths when completeSelection is true,
since CompleteValue is guaranteed to dereference mInput.)
not a topcrasher. if you all decide you're happy with a quick (and safe) fix,
please request approval on the patch and we'll evaluate. thanks.
Flags: blocking1.8b5+ → blocking1.8b5-
If this is a regression from 270697, then I'd have to say that those 
dereferences are safe, since we were doing them unconditionally before that 
patch landed (and after SelectBy was called).

If it's not a regression from that patch, then we might need more protection.  
I wasn't able to reproduce this on Windows and don't have a build handy on 
Linux, so I'm not able to verify right now whether the crash would still 
happen.

I can't reproduce the crash using my original steps either.
Attachment #197485 - Flags: second-review?(dbaron)
Attachment #197485 - Flags: second-review+
Attachment #197485 - Flags: first-review?(dbaron)
Attachment #197485 - Flags: first-review+
(In reply to comment #14)
> I can't reproduce the crash using my original steps either.

Then again, autocomplete doesn't actually work in today's build (it doesn't fill
in what's selected), so that could be the reason I don't see it.
Patch is checked in, please reopen if these crashes continue to show up.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
I can reproduce this fairly reliably on 1.5 on Windows, if I type something in the urlbar that starts a few autocomplete entries (i.e. "moz"), then start arrowing down.   It crashes at the point where the page loads and takes focus.  I'm attaching a stack that shows how mInput gets nulled out in the middle of HandleKeyNavigation.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
So, the real problem here relates to the fix for bug 192577.  The page loading takes focus away from the urlbar autocomplete, but the attribute that says that the popup is closed is on a 50ms timeout.  So, there's a window where we see the popup is "open" but it actually has just been closed (but by another input).

To fix this, I think it's important that everything related to closing the popup happen synchronously.  I tried backing out the fix that was checked in for bug 192577 and arrived at a somewhat different solution than what was used.  We really just need to make sure the rollup event is consumed if the popup was opened using the dropdown arrow.  To accomodate this, I added some functionality to PopupBoxObject that lets the XBL binding specify that the rollup event should be consumed.  (The reason that bug doesn't happen on Linux, btw, is because gtk2 widget completely ignores the aConsumeRollupEvent flag.)

There's a second problem which I decided to fix at the same time.  If you focus a different XUL textbox such as Firefox's search field, then click the urlbar dropdown arrow, the popup opens and then immediately closes.  The reason for this is that the two textfields actually use separate AutoCompleteController objects, so the search field happily thinks that it has the popup open, and closes it when it sees the blur (when in fact, the same popup is now being used by the urlbar).  The solution to this, I think, is to only use a single AutoCompleteController (all content-area textfields were already sharing a controller, but we need to do the same for XUL textboxes).  I did this by making the controller a service.

fwiw, I talked to Ben about the changes to the PopupBoxObject interface and I think we agreed that this is probably the best we can do without breaking compatibility.
Attachment #204385 - Flags: first-review?(roc)
This bug is obviously well understood, but including talkback anyway (win32, build 2005110712) ...

Incident ID: 12605599
Stack Signature	nsAutoCompleteController::HandleKeyNavigation fab8d461
Product ID	Firefox15
Build ID	2005110712
Trigger Time	2005-12-04 18:50:26.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	firefox.exe + (003fe63f)
URL visited	
User Comments	many windows open -- DAMN copied some text was scrolling through URL bar history while a tab was loading. s/vseerror
Since Last Crash	652842 sec
Total Uptime	652842 sec
Trigger Reason	Access violation
Source File, Line No.	c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp, line 400
Stack Trace 	
nsAutoCompleteController::HandleKeyNavigation  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp, line 400]
nsFormFillController::KeyPress  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/components/satchel/src/nsFormFillController.cpp, line 656]
DispatchToInterface  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventListenerManager.cpp, line 141]
nsEventListenerManager::HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventListenerManager.cpp, line 1779]
nsGenericElement::HandleDOMEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/base/src/nsGenericElement.cpp, line 2169]
nsHTMLInputElement::HandleDOMEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/html/content/src/nsHTMLInputElement.cpp, line 1395]
PresShell::HandleEventInternal  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp, line 6367]
PresShell::HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp, line 6203]
nsViewManager::HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/view/src/nsViewManager.cpp, line 2514]
nsViewManager::DispatchEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/view/src/nsViewManager.cpp, line 2246]
HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/view/src/nsView.cpp, line 174]
nsWindow::DispatchEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp, line 1252]
nsWindow::DispatchKeyEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp, line 3448]
nsWindow::OnKeyDown  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp, line 3586]
nsWindow::ProcessMessage  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp, line 4492]
nsWindow::WindowProc  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp, line 1434]
USER32.dll + 0x8734 (0x77d48734)
USER32.dll + 0x8816 (0x77d48816)
USER32.dll + 0x89cd (0x77d489cd)
USER32.dll + 0x8a10 (0x77d48a10)
nsAppShell::Run  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsAppShell.cpp, line 159]
nsAppStartup::Run  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp, line 151]
main  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/browser/app/nsBrowserApp.cpp, line 61]
kernel32.dll + 0x16d4f (0x7c816d4f)
Storing frame pointers in the popup box object scares me. That is just asking for crashes (possibly exploitable) if some nasty test case causes a reframe. I would be *much* happier if we just re-obtained the relevant frame pointers every time we need them. Is that possible?
(In reply to comment #22)
> Storing frame pointers in the popup box object scares me. That is just asking
> for crashes (possibly exploitable) if some nasty test case causes a reframe. I
> would be *much* happier if we just re-obtained the relevant frame pointers
> every time we need them. Is that possible?
> 

That's actually what it does now.  The only pointers stored in the popup box object are (all inherited from nsBoxObject):

  nsCOMPtr<nsIBoxLayoutManager> mLayoutManager; // [OWNER]
  nsCOMPtr<nsIBoxPaintManager> mPaintManager; // [OWNER]
  nsAutoPtr<nsPresState> mPresState; // [OWNER]

  nsIContent* mContent; // [WEAK]
  nsIPresShell* mPresShell; // [WEAK]
Comment on attachment 204385 [details] [diff] [review]
patch (against 1.5 branch, but should be portable to trunk)

Hmm, I'm not really sure what I was thinking. r+sr=roc
Attachment #204385 - Flags: first-review?(roc) → first-review+
*** Bug 321580 has been marked as a duplicate of this bug. ***
checked in on the trunk.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Comment on attachment 197485 [details] [diff] [review]
quick fix

I'd like to get this patch in for the next branch releases too, we're seeing a fair number of these crashes in talkback.
Attachment #197485 - Flags: approval1.8.1?
Attachment #197485 - Flags: approval1.8.0.1?
Comment on attachment 204385 [details] [diff] [review]
patch (against 1.5 branch, but should be portable to trunk)

This patch is needed as well to completely fix the crashes.

I'll go ahead and call out the interface change to nsIPopupBoxObject.  This is an unfrozen interface, and all I'm doing here is adding a new method.  So, there should be no impact to script callers of the interface, but C++ callers would need to recompile due to the IID change.
Attachment #204385 - Flags: approval1.8.1?
Attachment #204385 - Flags: approval1.8.0.1?
Comment on attachment 204385 [details] [diff] [review]
patch (against 1.5 branch, but should be portable to trunk)

I'm going to post a new version of this for 1.8.0.1 that doesn't change any interfaces.  I'd still like to go with this version for the 1.8.1 / Firefox 2 branch.
Attachment #204385 - Flags: approval1.8.0.1?
Comment on attachment 204385 [details] [diff] [review]
patch (against 1.5 branch, but should be portable to trunk)

Actually, we _don't_ need this pne at all just to fix the crashes.  The original patch here is sufficient for that, and I think that's the only one we should take for 1.8.0.1.
This is a topcrasher for Firefox 1.5 and we should take this for 1.8.0.1 if  possible, so nominating.

Thanks bryner for getting this patch together and checked in on the Trunk.  It has been around for a while...so nice to see another crasher fixed. :-)
Flags: blocking1.8.0.1?
Comment on attachment 197485 [details] [diff] [review]
quick fix

a=dveditz. please add fixed1.8.0.1 and fixed1.8.1 keywords when checked in
Attachment #197485 - Flags: approval1.8.1?
Attachment #197485 - Flags: approval1.8.1+
Attachment #197485 - Flags: approval1.8.0.1?
Attachment #197485 - Flags: approval1.8.0.1+
Flags: blocking1.8.0.1? → blocking1.8.0.1+
checked in on branches
Keywords: fixed1.8.0.1, fixed1.8.1
v.fixed on 1.8.0.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060104 Firefox/1.5.0.1.  Unable to crash using the dbaron's steps in comment #0.
Keywords: fixed1.8.0.1 → verified1.8.0.1
Keywords: fixed1.8.0.1
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.1, qawanted
Comment on attachment 204385 [details] [diff] [review]
patch (against 1.5 branch, but should be portable to trunk)

Can't take interface changes to nsIPopupBoxObject on the 1.8 branch (and probably shouldn't for the nsIMenuParent pseudo-interface either).
Attachment #204385 - Flags: approval1.8.1? → approval1.8.1-
Depends on: 404135
Crash Signature: [@ nsAutoCompleteController::HandleKeyNavigation]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: