Closed Bug 1406229 Opened 2 years ago Closed 2 years ago

Firefox crashes with autocomplete/formfill popup in browserAction

Categories

(WebExtensions :: Untriaged, defect, P1, major)

x86_64
macOS
defect

Tracking

(firefox-esr52 unaffected, firefox56 wontfix, firefox57+ fixed, firefox58 verified)

VERIFIED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 + fixed
firefox58 --- verified

People

(Reporter: mythmon, Assigned: mixedpuppy)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

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.
I can also reproduce this on Firefox 57 Beta 5 with the same STR.
Adding crash signature to the bug. Jorge - do you think we can contact Dan, the add on author regarding this crash?
Crash Signature: [@ nsAutoCompleteController::EnterMatch]
Flags: needinfo?(jorge)
Dan, can you please look into this issue? Also moving to WE components, since it could be an API bug.
Component: General → WebExtensions: Untriaged
Flags: needinfo?(jorge) → needinfo?(dan.callahan)
Product: Firefox → Toolkit
A webextension shouldn't be able to crash Firefox, this isn't on Dan to fix.
Priority: -- → P1
Looks like this is a problem with our <datalist> handling. I guess somehow mInput is getting set to null somewhere between HandleEnter and EnterMatch.
I bet it has something to do with this in popup.js:

  input.addEventListener('keydown', e => {
    if (e.key === "Enter") { window.close(); }
  });
Clearing ni based on Comment 4.
Flags: needinfo?(dan.callahan)
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.
I'm investigating further.
Assignee: nobody → mixedpuppy
Blocks: 1406585
Keywords: crash
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.
Attachment #8916189 - Flags: review?(mak77) → review+
(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.
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/118b67c940dc
fix autocomplete crash in panels when window is closed during event, r=mak
https://hg.mozilla.org/mozilla-central/rev/118b67c940dc
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Attached image 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.
Status: RESOLVED → VERIFIED
How was this verified on Beta when the patch hasn't been uplifted there yet?!
Flags: needinfo?(marius.santa)
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.
I've verified that nightly still crashed prior to pulling m-c changes (that include this bugs patch).  Now it no longer crashes.
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.
Attachment #8916189 - Flags: approval-mozilla-beta?
Summary: Firefox crashes with webextension Quick Accept-Language Switcher → Firefox crashes with autocomplete/formfill popup in browserAction
Blocks: 1407329
Comment on attachment 8916189 [details]
Bug 1406229 fix autocomplete crash in panels when window is closed during event,

low risk, crash fix, beta57+
Attachment #8916189 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Flags: needinfo?(marius.santa) → needinfo?(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.
Flags: needinfo?(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.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.