Closed Bug 1166932 Opened 6 years ago Closed 6 years ago
[e10s]&[non-e10s] - all tabs crash when quickly typing several chars into Facebook's "To:" messenger field
294.44 KB, image/gif
1.54 KB, patch
|Details | Diff | Splinter Review|
Part 2: Make sure that nsTextInputListener doesn't try to perform edit actions if it has been disconnected from the editor object
983 bytes, patch
|Details | Diff | Splinter Review|
Selecting the "To:" field under Facebook's messenger and quickly typing in several characters right after you've loaded the page will crash all the tabs that are currently opened in fx. I can reproduce this consistently and added a quick .gif to illustrate the crash. (in the gif, I quickly select the "To:" field and type in "sonj") OS's: ----- * OSX 10.10.3 x64 - Reproduced several times * Win 8.1 x64 VM - Reproduced several times STR: ---- - login into facebook and select "Messenger -> See All" - Select "New Message" - as soon as the page loads, quickly select the "To:" field and start typing in a name (should instantly crash) Might be related to bug # 1165369 as I also got the same crash report once when reproducing this: (the three listed below are more common) * https://crash-stats.mozilla.com/report/index/fc94ce3d-fd40-4e87-b451-24b342150520 Crash Reports: -------------- - https://crash-stats.mozilla.com/report/index/c94af882-c8dd-4835-924c-fa5a02150520 [OSX] - https://crash-stats.mozilla.com/report/index/be243602-a4a0-4a65-9397-ef2b72150520 [Windows]
Does the issue happen on FF40 too?
(In reply to Olli Pettay [:smaug] from comment #1) > Does the issue happen on FF40 too? Yup, reproduced it with fx40 using the same steps that are listed above. - Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-05-20-00-40-04-mozilla-aurora/ - Crash: https://crash-stats.mozilla.com/report/index/e420af19-13a7-4fcd-9062-a85452150520
Quickly checked to see if bug # 1166742 might have fixed the crash, unfortunately it's still crashing with today's m-c. - Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-05-21-03-02-04-mozilla-central/ - Crash: https://crash-stats.mozilla.com/report/index/3177a6f5-3ef7-419e-90ac-0698f2150521
[Tracking Requested - why for this release]: The nsEditor::NotifyEditorObservers signature is #2 in yesterday's Dev Edition 40 data.
I can reproduce the crash on Nightly41.0a1(201-05-21) without e10s. Browser crashes with crash reporter. bp-91965820-e5c5-4d19-9514-c1dba2150521
Summary: [e10s] - all tabs crash when quickly typing several chars into Facebook's "To:" messenger field → [e10s]&[non-e10s] - all tabs crash when quickly typing several chars into Facebook's "To:" messenger field
(In reply to Kamil Jozwiak [:kjozwiak] from comment #2) > (In reply to Olli Pettay [:smaug] from comment #1) > > Does the issue happen on FF40 too? > > Yup, reproduced it with fx40 using the same steps that are listed above. So very much looks like a regression from bug 1154701. I assume one of those for (i = 0; i < array.length; ++i) -> for (item: array) changes caused this. those are rather different ways to iterate array, especially from security point of view.
This is very likely a use after free.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7) > This is very likely a use after free. And it is. We're accessing what an iterator points to after we mutate the array to remove the item.
Comment on attachment 8608951 [details] [diff] [review] Part 1: Copy the editor observers array before iterating over it (Note: these approvals are being requested for both of the patches here.) [Security approval request comment] How easily could an exploit be constructed based on the patch? The first patch makes the issue pretty obvious, but it's impossible to fix this issue in another way. It is not easy to go from the patch to an exploit, however. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? See above. Which older supported branches are affected by this flaw? Only Nightly and Aurora. If not all supported branches, which bug introduced the flaw? Bug 1154701 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I'd prefer to fix this, the backout may be regression prone. How likely is this patch to cause regressions; how much testing does it need? It technically changes the behavior for running the edit actions, but I don't expect it to be very regression prone. At any rate, we do have some baking time. Approval Request Comment [Feature/regressing bug #]: Bug 1154701. [User impact if declined]: Comment 0. [Describe test coverage new/current, TreeHerder]: Locally tested. [Risks and why]: See above. [String/UUID change made/needed]: None.
Attachment #8608951 - Flags: review?(roc) → review+
Attachment #8608952 - Flags: review?(roc) → review+
Jesse, maybe some fuzzing coverage is needed here?
Comment on attachment 8608951 [details] [diff] [review] Part 1: Copy the editor observers array before iterating over it Checkin all the things!
(Replying bug 1162360 comment 41) I think I was indeed trying to solve the same issue there, but just by reading the descriptions I don't know if these patches work for my use case. I explicitly choose not to copy the array before iterating it because we do need to notify the observers added while the loop is being run on. Should we instead landing my patch in this bug? Anyhow, I am going to read the patches here and try them too on Monday.
> I explicitly choose not to copy the array before iterating it because we do need to notify > the observers added while the loop is being run on. I agree with this.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (working slowly due to bad health condition) from comment #17) > > I explicitly choose not to copy the array before iterating it because we do need to notify > > the observers added while the loop is being run on. > > I agree with this. Which observers do we need to run? If you know of one where this will end up doing the wrong behavior, please file a new bug for it and needinfo me on it so that we can discuss that. Thanks!
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #18) > Which observers do we need to run? If you know of one where this will end > up doing the wrong behavior, please file a new bug for it and needinfo me on > it so that we can discuss that. Thanks! Let me re-phrase bug 1162360 comment 11: On B2G, we would have 3 observers in the observers array 0: nsTextInputListener 1: FormAssistant (in forms.js) 2: IMEContentObserver The part 1 patch of bug 1162360 is applied, FormAssistant.EditAction will eventually cause IMEContentObserver at 2 to be destroyed and a new one added being assigned in its place. A range-based for loop will attempt to call on the old one, hence the crash. we will ask RemoveEditorObserver do things differently: when the old one got replaced, we will replace it with a nullptr so it still keep its place. The newly added IMEContentObserver will be assigned in the 3rd place. 0: nsTextInputListener 1: FormAssistant (in forms.js) 2: nullptr (IMEContentObserver removed cause by action in FormAssistant) 3: IMEContentObserver (added cause by action in FormAssistant) And a traditional for loop will ensure we'll always run to to the very end so the added one gets notified. nullptr will get cleaned up once the loop is finished. The first patch here (copying the array) will not prevent the crash in the case mentioned, and it will miss the newly added IMEContentObserver.
Ehsan, could you respond to comment 19 and advice how to move bug 1162360 forward since this is landed? Should I just rebase that patch against this one?
I was wrong at comment 19; after pulling the latest m-c and include the fix here, the patch there no longer crash the emulator on test. I am still not sure if the patch here is the right behavior though, but without a test failure I would like to land the first patch once the depended gaia test fixes is merged. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e59f08d4cb79
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #19) > The first patch here (copying the array) will not prevent the crash in the > case mentioned, and it will miss the newly added IMEContentObserver. The problem with your patch #2 on that bug is that it will cause us to call EditAction() on observers that were not registered at the time that the editing operation happened, which is incorrect in the general case for other observers. Couldn't you call the EditAction manually when you register the observer as part of another observer's EditAction()? (FWIW, we can move this discussion to bug 1162360, I think.)
Went through verification using the following builds: - http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-05-26-03-02-02-mozilla-central/ - http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-05-26-00-40-04-mozilla-aurora/ Tested with the following OS's: - Win 8.1 x64 VM - PASSED - OSX 10.10.3 - PASSED - Ubuntu 14.0 - PASSED I can't reproduce the crash anymore, went through both m-c and m-a on the above OS's without any problems. Ran through the case in comment #0 several times without any problems.
You need to log in before you can comment on or make changes to this bug.