1406229 - Firefox crashes with autocomplete/formfill popup in browserAction

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Michael Cooper [:mythmon]]

STR

1. Install "Quick Accept-Language Switcher": https://addons.mozilla.org/en-US/firefox/addon/quick-accept-language-switc/

2. Click on the extension's toolbar icon.

3. Type in "en-US" and press enter.

Expected:

Browser's Accept-Language header changes.

Actual:

Browser crashes. Here is an example crash: https://crash-stats.mozilla.com/report/index/984c488e-de6f-462c-af4f-5492b0171005

This is reproducible 100% of the time on a fresh profile.

Comment 1

[Michael Cooper [:mythmon]]

I can also reproduce this on Firefox 57 Beta 5 with the same STR.

Comment 2

[Marcia Knous [:marcia]]

Adding crash signature to the bug. Jorge - do you think we can contact Dan, the add on author regarding this crash?

Comment 3

[Jorge Villalobos [:jorgev] (he/him)]

Dan, can you please look into this issue? Also moving to WE components, since it could be an API bug.

Comment 4

[Andy McKay]

A webextension shouldn't be able to crash Firefox, this isn't on Dan to fix.

Comment 5

[Kris Maglione [:kmag]]

Looks like this is a problem with our <datalist> handling. I guess somehow mInput is getting set to null somewhere between HandleEnter and EnterMatch.

Comment 6

[Kris Maglione [:kmag]]

I bet it has something to do with this in popup.js:

  input.addEventListener('keydown', e => {
    if (e.key === "Enter") { window.close(); }
  });

Comment 7

[Marcia Knous [:marcia]]

Clearing ni based on Comment 4.

Comment 8

[Shane Caraveo (:mixedpuppy)]

The cause is not specific to webextensions, and the crash could probably happen in other non-WE situations, but it's most likely to happen via extensions.  

Running popup.html in a tab, the tab closes but the datalist remains open.  It's not crashing presumably because the window that owns the datalist popup is the browser window in this case, so it still exists.

The crash specifically happens in nsAutoCompleteController::EnterMatch.

Comment 9

[Shane Caraveo (:mixedpuppy)]

I'm investigating further.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1406229 fix autocomplete crash in panels when window is closed during event,

Review commit: https://reviewboard.mozilla.org/r/187436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187436/

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8916189 [details]
Bug 1406229 fix autocomplete crash in panels when window is closed during event,

https://reviewboard.mozilla.org/r/187436/#review192668

Thank you!
There is another case in HandleStartComposition where we call StopSearch() and then immediately deref input, could you please also fix that one while here?

In general the situation with mInput is quite problematic, since potentially anything can null-out mInput (the setter is exposed, once could use that setter from any handler). This is not the first nor it will be the last crash related to mInput. We need some kind of long term solution, maybe we should never nullify mInput, but rather have an inputIsValid bool. No idea, but worth brainstorming for the future.

Comment 12

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [::mak] from comment #11)
> There is another case in HandleStartComposition where we call StopSearch()
> and then immediately deref input, could you please also fix that one while
> here?

Ah, nevermind, that is using a local input reference, so it should be fine.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/118b67c940dc
fix autocomplete crash in panels when window is closed during event, r=mak

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/118b67c940dc

Comment 15

[marius.santa]

Attachment: no crash en-US.gif

This bug is verified on Firefox 57.0b7 (20171009192146) and Firefox 58.0a1 (20171010100200) under Windows 10 64bit/Mac OS X 10.12.3.

Please see the attached video.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

How was this verified on Beta when the patch hasn't been uplifted there yet?!

Comment 17

[Shane Caraveo (:mixedpuppy)]

I'm unable to repro the crash in 57 beta 7.  

It is a timing issue, so while I was able to repro 100% on 58, not entirely surprised its harder on 57.  I still think this is worth uplifting as it does add a protection there and the code change is simple.  It was also produced on beta 5 per comment 1.

Comment 18

[Shane Caraveo (:mixedpuppy)]

I've verified that nightly still crashed prior to pulling m-c changes (that include this bugs patch).  Now it no longer crashes.

Comment 19

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8916189 [details]
Bug 1406229 fix autocomplete crash in panels when window is closed during event,

Approval Request Comment
[Feature/Bug causing the regression]: autcomplete/formfill popups
[User impact if declined]: crash
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: I have, QA should
[Needs manual test from QE? If yes, steps to reproduce]: STR is in comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: It is a minimal change to exit a call early.
[String changes made/needed]: none

While I was unable to repro in beta 7, I do think this should be uplifted as it is timing related.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916189 [details]
Bug 1406229 fix autocomplete crash in panels when window is closed during event,

low risk, crash fix, beta57+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/09655131b7e8

Comment 22

[marius.santa]

I was unable to reproduce the issue caused by https://addons.mozilla.org/en-US/firefox/addon/quick-accept-language-switc/ in Firefox 57.0b7 (20171009192146) or Firefox 57.0b8 (20171013042429).
I am not able to reproduce the crash caused with "autocomplete/formfill: on any of the builds, please provide some steps to reproduce.

Comment 23

[Shane Caraveo (:mixedpuppy)]

The STR in comment 0 worked (ie. crashed) 100% for me, not even an intermittent crash.  I don't think I can add anything further to it.

Comment 24

[Shane Caraveo (:mixedpuppy)]

The one thing I can add, it may be timing related so if your machine has different characteristics, perhaps that would be the reason you cannot reproduce.  Just a guess.