Closed Bug 433788 Opened 12 years ago Closed 5 years ago

Crash in [@ nsAutoCompleteController::ClosePopup] due to re-entrancy

Categories

(Toolkit :: Autocomplete, defect, critical)

x86
Windows Vista
defect
Not set
critical
Points:
5

Tracking

()

RESOLVED FIXED
mozilla34
Iteration:
34.1

People

(Reporter: bruno, Assigned: mak)

References

Details

(Keywords: crash, Whiteboard: [ccbr])

Crash Data

Attachments

(2 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.14) Gecko/20080501 Firefox/2.0.0.14 Flock/1.2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.14)

I was trying to register a new webex user, going back and forth between the registration pages and the following crash happened.

Note: mInput was tested for null a few lines before but got null (threading problem?)

Reproducible: Couldn't Reproduce
I was using a FF3 product
Attached image crash stack
Bug 384879 fixed a bunch of these cases, but didn't touch ClosePopup(). It's not clear to me how mInput can be destroyed within that method.
Component: General → Autocomplete
Product: Firefox → Toolkit
QA Contact: general → autocomplete
Er, didn't see your screenshot before. There's definitely something wacky going on if the *first* de-reference fails, because there's no threading involved here.
I had the following exception happening and our product displays it in a popup (that could explain the timing pb):

JavaScript error: , line 0: uncaught exception: [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: chrome://global/content/bindings/autocomplete.xml :: onxblpopuphiding :: line 834"  data: no]
these crashes aren't threading, they're reentrancy. threading isn't an issue because these objects will crash or deadlock instantly with fairly obvious stacks if someone tries to touch them from another thread.
reporter: text based stack traces are appreciated. 
http://developer.mozilla.org/en/docs/How_to_get_a_stacktrace_with_WinDbg
is one approach as it makes text based stack traces (and complex instructions) easy.

keep in mind that my patch didn't hit the branch.

trunk:
966 nsAutoCompleteController::ClosePopup()
967 {
968   if (!mInput) {
969     return NS_OK;
970   }
971 
972   PRBool isOpen;
973   mInput->GetPopupOpen(&isOpen);
974   if (!isOpen)
975     return NS_OK;
976 
977   nsCOMPtr<nsIAutoCompletePopup> popup;
978   mInput->GetPopup(getter_AddRefs(popup));

the crash is on 978, the reentrancy is under 973. you can write a patch based on how i wrote mine in the bug gavin referenced or wait for me to write one.
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash
Summary: Crash in nsAutoCompleteController::ClosePopup → Crash in [@ nsAutoCompleteController::ClosePopup]
Version: unspecified → Trunk
Resolving as Invalid, because this is caused by a combination of bug 428602 and displaying js errors in dialog boxes, so it won't happen in real-cases.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
no. the bug's valid.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
i've seen 2 crashes today ending in nsAutoCompleteController::ClosePopup but stack crash looks different

http://crash-stats.mozilla.com/report/index/d67e88a5-3af1-4c95-8991-c4d6b2090414
http://crash-stats.mozilla.com/report/index/ed22f8f1-8aa4-4e96-8b4d-d8d6b2090414

0  	xul.dll  	nsAutoCompleteController::ClosePopup  	toolkit/components/autocomplete/src/nsAutoCompleteController.cpp:970
1 	xul.dll 	nsAutoCompleteController::ProcessResult 	toolkit/components/autocomplete/src/nsAutoCompleteController.cpp:1260
2 	xul.dll 	nsAutoCompleteController::OnSearchResult 	toolkit/components/autocomplete/src/nsAutoCompleteController.cpp:684
that was in build Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre
marco, that crash is bug 488311 (well, they're all really the same, and someone needs to fix it for trunk, but if you want a branch fix for your branch build, then the bug i've referenced has done that).
Bug 488311 is fixed, but I'm guessing that's just a band-aid for the real problem in bug 488812.
Summary: Crash in [@ nsAutoCompleteController::ClosePopup] → Crash in [@ nsAutoCompleteController::ClosePopup] due to re-entrancy
Whiteboard: [ccbr]
Crash Signature: [@ nsAutoCompleteController::ClosePopup]
Do bug 660556, bug 756861 and bug 720589 have the same root cause?


As for current crash sigs, I examined 6 crashes. Stack A below has the same stack as comment 2

stack A
978  mInput->GetPopupOpen(&isOpen);
http://hg.mozilla.org/releases/mozilla-beta/annotate/c714ac7fb431/toolkit/components/autocomplete/nsAutoCompleteController.cpp#l978

bp-be9158db-7ee2-42a8-bedd-9eebf2140509
bp-296ef36f-8fdc-49a5-ae63-86c922140509
0	xul.dll	nsAutoCompleteController::ClosePopup()	toolkit/components/autocomplete/nsAutoCompleteController.cpp
1	xul.dll	nsAutoCompleteController::SetInput(nsIAutoCompleteInput *)	toolkit/components/autocomplete/nsAutoCompleteController.cpp
2	xul.dll	nsFormFillController::StopControllingInput()	toolkit/components/satchel/nsFormFillController.cpp 

Two other stacks, B and C, do not crash at the same line as stack A. But B and C crash on the same line
986  return mInput->SetPopupOpen(false);
http://hg.mozilla.org/releases/mozilla-release/annotate/f60bc49e6bd5/toolkit/components/autocomplete/nsAutoCompleteController.cpp#l986

stack B
bp-c406f6bc-5a96-4c30-af10-175452140411
bp-d08d0224-6d69-426a-afaf-249692140420
0	xul.dll	nsAutoCompleteController::ClosePopup()	toolkit/components/autocomplete/nsAutoCompleteController.cpp
1	xul.dll	nsAutoCompleteController::PostSearchCleanup()	toolkit/components/autocomplete/nsAutoCompleteController.cpp
2	xul.dll	nsAutoCompleteController::StopSearch()	toolkit/components/autocomplete/nsAutoCompleteController.cpp  

stack C
bp-53cd1a87-1045-4bec-9dca-d2c6f2140425
bp-d9e1d555-ab81-49f8-893f-4ac2f2140712
0	xul.dll	nsAutoCompleteController::ClosePopup()	toolkit/components/autocomplete/nsAutoCompleteController.cpp
1	xul.dll	nsAutoCompleteController::EnterMatch(bool)	toolkit/components/autocomplete/nsAutoCompleteController.cpp
2	xul.dll	nsAutoCompleteController::HandleEnter(bool,bool *)	toolkit/components/autocomplete/nsAutoCompleteController.cpp
Crash Signature: [@ nsAutoCompleteController::ClosePopup] → [@ nsAutoCompleteController::ClosePopup] [@ nsAutoCompleteController::ClosePopup()]
Flags: needinfo?(gavin.sharp)
Whiteboard: [ccbr] → [ccbr][blocked on bug 488812]
Flags: needinfo?(gavin.sharp) → needinfo?(mak77)
Yes, the last 2 look like re-entrancy, so might be fixed by a patch here. The first one is exactly the same stack reported here, so it's the same. I'm not sure I can explain the first case with re-entrancy, though IIRC on some platforms the stack might be off by a couple rows in the source and we might actually be crashing later

There are other code points where we should add protection on mInput in the code. I don't think we should wait for bug 488812 to handle these crashes, we can start by fixing them.
Assignee: nobody → mak77
Status: REOPENED → ASSIGNED
Flags: needinfo?(mak77)
Marco, could you please add this to the current iteration?
Points: --- → 5
Flags: firefox-backlog+
Added to Iteration 34.1
Iteration: --- → 34.1
QA Whiteboard: [qa?]
QA Whiteboard: [qa?] → [qa-]
Whiteboard: [ccbr][blocked on bug 488812] → [ccbr]
Attached patch patch v1Splinter Review
this trivial patch might improve things, it's mostly continuing on the path we started to avoid issues with mInput. bug 488812 remains valid.
Attachment #8464706 - Flags: review?(enndeakin)
Attachment #8464706 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/integration/fx-team/rev/cbe49d980c05
Target Milestone: --- → mozilla34
https://hg.mozilla.org/mozilla-central/rev/cbe49d980c05
Status: ASSIGNED → RESOLVED
Closed: 12 years ago5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1117575
You need to log in before you can comment on or make changes to this bug.