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

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Bruno Calvignac]

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

Comment 1

[Bruno Calvignac]

I was using a FF3 product

Comment 2

[Bruno Calvignac]

Attachment: crash stack

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 5

[Bruno Calvignac]

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]

Comment 6

[:Gavin Sharp [email: [e-mail]]]

That's bug 428602.

Comment 7

[timeless]

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.

Comment 8

[timeless]

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.

Comment 9

[Bruno Calvignac]

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.

Comment 10

[timeless]

no. the bug's valid.

Comment 11

[Marco Bonardo [:mak]]

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

Comment 12

[Marco Bonardo [:mak]]

that was in build Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre

Comment 13

[timeless]

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).

Comment 14

[Jesse Ruderman on Windows]

Bug 488311 is fixed, but I'm guessing that's just a band-aid for the real problem in bug 488812.

Comment 15

[Wayne Mery (:wsmwk)]

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

Comment 16

[Marco Bonardo [:mak]]

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.

Comment 17

[Marco Bonardo [:mak]]

Marco, could you please add this to the current iteration?

Comment 18

[Marco Mucci [:MarcoM]]

Added to Iteration 34.1

Comment 19

[Marco Bonardo [:mak]]

Attachment: patch v1

this trivial patch might improve things, it's mostly continuing on the path we started to avoid issues with mInput. bug 488812 remains valid.

Comment 20

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/fx-team/rev/cbe49d980c05

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cbe49d980c05