1836806 - Assertion failure: IsValid(), at /builds/worker/checkouts/gecko/widget/ContentCache.cpp:234

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing m-c 20230602-8c3b1c60fde7 (--enable-address-sanitizer --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch -d --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html

Assertion failure: IsValid(), at /builds/worker/checkouts/gecko/widget/ContentCache.cpp:234

#0 0x7fc00fefabea in mozilla::ContentCacheInChild::CacheCaret(nsIWidget*, mozilla::widget::IMENotification const*) /builds/worker/checkouts/gecko/widget/ContentCache.cpp:234:3
#1 0x7fc00fefd612 in mozilla::ContentCacheInChild::SetSelection(nsIWidget*, mozilla::widget::IMENotification::SelectionChangeDataBase const&) /builds/worker/checkouts/gecko/widget/ContentCache.cpp:649:3
#2 0x7fc00ff17aa2 in mozilla::widget::PuppetWidget::NotifyIMEOfSelectionChange(mozilla::widget::IMENotification const&) /builds/worker/checkouts/gecko/widget/PuppetWidget.cpp:837:7
#3 0x7fc00ff2781f in mozilla::widget::TextEventDispatcher::NotifyIME(mozilla::widget::IMENotification const&) /builds/worker/checkouts/gecko/widget/TextEventDispatcher.cpp:486:40
#4 0x7fc00feef34a in nsBaseWidget::NotifyIME(mozilla::widget::IMENotification const&) /builds/worker/checkouts/gecko/widget/nsBaseWidget.cpp:1897:43
#5 0x7fc00e3bfde4 in mozilla::IMEStateManager::NotifyIME(mozilla::widget::IMENotification const&, nsIWidget*, mozilla::dom::BrowserParent*) /builds/worker/checkouts/gecko/dom/events/IMEStateManager.cpp
#6 0x7fc00e3c745d in mozilla::IMEContentObserver::IMENotificationSender::SendSelectionChange() /builds/worker/checkouts/gecko/dom/events/IMEContentObserver.cpp:1930:3
#7 0x7fc00e3c6419 in mozilla::IMEContentObserver::IMENotificationSender::Run() /builds/worker/checkouts/gecko/dom/events/IMEContentObserver.cpp:1753:7
#8 0x7fc0102f2680 in nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsRefreshDriver::IsExtraTick) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:2522:13
#9 0x7fc0102fc201 in TickDriver /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:373:13
#10 0x7fc0102fc201 in mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver>>&) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:351:7
#11 0x7fc0102fc100 in mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:367:5
#12 0x7fc0102fbf9d in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:911:5
#13 0x7fc0102fb316 in mozilla::VsyncRefreshDriverTimer::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:825:5
#14 0x7fc0102fa649 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsyncTimerOnMainThread() /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:592:14
#15 0x7fc00f68014b in mozilla::dom::VsyncMainChild::RecvNotify(mozilla::VsyncEvent const&, float const&) /builds/worker/checkouts/gecko/dom/ipc/VsyncMainChild.cpp:66:15
#16 0x7fc00f94ea2e in mozilla::dom::PVsyncChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PVsyncChild.cpp:220:78
#17 0x7fc00f840b80 in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PContentChild.cpp:8770:32
#18 0x7fc00b62e69f in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1811:25
#19 0x7fc00b62b3f2 in mozilla::ipc::MessageChannel::DispatchMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::UniquePtr<IPC::Message, mozilla::DefaultDelete<IPC::Message>>) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1736:9
#20 0x7fc00b62c072 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1536:3
#21 0x7fc00b62d1bf in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1634:14
#22 0x7fc00a966417 in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:555:16
#23 0x7fc00a9614ca in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:879:26
#24 0x7fc00a95fe37 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:702:15
#25 0x7fc00a960295 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:491:36
#26 0x7fc00a969a86 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:218:37
#27 0x7fc00a969a86 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_0>::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548:5
#28 0x7fc00a980a0a in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1240:16
#29 0x7fc00a9878bd in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:479:10
#30 0x7fc00b6346d5 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:85:21
#31 0x7fc00b550541 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:361:3
#32 0x7fc00b550541 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:343:3
#33 0x7fc00ff475d8 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148:27
#34 0x7fc01224da8b in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:724:20
#35 0x7fc00b6355b6 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:235:9
#36 0x7fc00b550541 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:361:3
#37 0x7fc00b550541 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:343:3
#38 0x7fc01224d352 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:659:34
#39 0x561aef08b526 in content_process_main /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
#40 0x561aef08b526 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:375:18
#41 0x7fc020429d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#42 0x7fc020429e3f in __libc_start_main csu/../csu/libc-start.c:392:3
#43 0x561aef0627c8 in _start (/home/user/workspace/browsers/m-c-20230605094751-fuzzing-debug/firefox-bin+0x587c8) (BuildId: 12ad878cfaa70b2bc4a7191a0344fcaba161fd13)

Comment 1

[Mayank Bansal]

Got a crash from the testcase on Nightly: https://crash-stats.mozilla.org/report/index/8864decb-8adb-4c9c-918f-83f4c0230606#tab-bugzilla

Comment 2

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20230608152955-256876c3862b.

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

The assertion was added in bug 1836806.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Warnings before the assertion failure:

[Child 31944, Main Thread] WARNING: NS_ENSURE_TRUE(mPresShell) failed: file M:/src/layout/generic/nsFrameSelection.cpp:1711
[Child 31944, Main Thread] WARNING: '!mBoundFrame', file M:/src/dom/html/TextControlState.cpp:2353
[Child 31944, Main Thread] WARNING: '!mSelection->IsValidIn(*mText)', file M:/src/widget/ContentCache.cpp:53
Assertion failure: IsValid(), at M:/src/widget/ContentCache.cpp:234

So, ContentCacheInChild::mText seems outdated.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

[Child 38704, Main Thread] WARNING: '!mBoundFrame', file M:/src/dom/html/TextControlState.cpp:2353
[Child 38704: Main Thread]: V/IMEContentObserver 0x17c1a4f9c10 MaybeNotifyIMEOfPositionChange()
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10 PostPositionChangeNotification()
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10   IsSafeToNotifyIME(), it's not safe because of no widget
[Child 38704: Main Thread]: W/IMEContentObserver 0x17c1a4f9c10   FlushMergeableNotifications(), Warning, do nothing due to unsafe to notify IME
================================== HTMLInputElement::ApplyStep() is called here
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10 BeginDocumentUpdate(), HasAddedNodesDuringDocumentChange()=false
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10 EndDocumentUpdate(), HasAddedNodesDuringDocumentChange()=false
[Child 38704: Main Thread]: V/IMEContentObserver 0x17c1a4f9c10 MaybeNotifyIMEOfPositionChange()
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10 PostPositionChangeNotification()
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10   IsSafeToNotifyIME(), it's not safe because of no widget
[Child 38704: Main Thread]: W/IMEContentObserver 0x17c1a4f9c10   FlushMergeableNotifications(), Warning, do nothing due to unsafe to notify IME
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10 BeginDocumentUpdate(), HasAddedNodesDuringDocumentChange()=false
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10 EndDocumentUpdate(), HasAddedNodesDuringDocumentChange()=false
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10   IsSafeToNotifyIME(), it's not safe because of no widget
[Child 38704: Main Thread]: W/IMEContentObserver 0x17c1a4f9c10   FlushMergeableNotifications(), Warning, do nothing due to unsafe to notify IME
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10 UnsuppressNotifyingIME(), mSuppressNotifications=1
[Child 38704: Main Thread]: W/IMEContentObserver 0x17c1a4f9c10   FlushMergeableNotifications(), Warning, do nothing due to already flushing pending notifications
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10 MaybeNotifyIMEOfSelectionChange(aCausedByComposition=false, aCausedBySelectionEvent=false, aOccurredDuringComposition)
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10 PostSelectionChangeNotification(), mSelectionData={ mCausedByComposition=false, mCausedBySelectionEvent=false }
[Child 38704: Main Thread]: W/IMEContentObserver 0x17c1a4f9c10   FlushMergeableNotifications(), Warning, do nothing due to already flushing pending notifications
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10 MaybeNotifyIMEOfSelectionChange(aCausedByComposition=false, aCausedBySelectionEvent=false, aOccurredDuringComposition)
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10 PostSelectionChangeNotification(), mSelectionData={ mCausedByComposition=false, mCausedBySelectionEvent=false }
[Child 38704: Main Thread]: W/IMEContentObserver 0x17c1a4f9c10   FlushMergeableNotifications(), Warning, do nothing due to already flushing pending notifications
[Child 38704: Main Thread]: V/IMEContentObserver 0x17c1a4f9c10 MaybeNotifyIMEOfPositionChange()
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10 PostPositionChangeNotification()
[Child 38704: Main Thread]: W/IMEContentObserver 0x17c1a4f9c10   FlushMergeableNotifications(), Warning, do nothing due to already flushing pending notifications
[Child 38704: Main Thread]: D/IMEContentObserver 0x17c1a4f9c10 UpdateSelectionCache(), mSelectionData={ mOffset=1, mString="" (Length()=0), GetWritingMode()=h-ltr, mReversed=false, mCausedByComposition=false, mCausedBySelectionEvent=false, mOccurredDuringComposition=false }
[Child 38704: Main Thread]: I/IMEContentObserver 0x17c006a7380 IMENotificationSender::SendSelectionChange(), sending NOTIFY_IME_OF_SELECTION_CHANGE... newSelChangeData={ mOffset=1, mString="" (Length()=0), GetWritingMode()=h-ltr, mReversed=false, mCausedByComposition=false, mCausedBySelectionEvent=false, mOccurredDuringComposition=false }
[Child 38704: Main Thread]: I/ContentCacheWidgets 0x17c15996640 SetSelection(aSelectionChangeData={ mOffset=1, mString="" (Length()=0), GetWritingMode()=h-ltr, mReversed=false, mCausedByComposition=false, mCausedBySelectionEvent=false, mOccurredDuringComposition=false }), mText="" (Length()=0)
[Child 38704: Main Thread]: I/ContentCacheWidgets 0x17c15996640 CacheCaret(aWidget=0x17c15996400, aNotification=Not notification)
[Child 38704: Main Thread]: I/IMEContentObserver 0x17c1a4f9c10 HandleQueryContentEvent(aEvent={ mMessage=eQueryCaretRect })
[Child 38704: Main Thread]: I/ContentCacheWidgets 0x17c15996640   CacheCaret(), Succeeded, mSelection={ mAnchor=1, mFocus=1, mWritingMode=h-ltr, Reversed()=false, StartOffset()=1, EndOffset()=1, IsCollapsed()=true, Length()=0 }, mCaret={ mOffset=1, mRect=(x=38, y=22, w=3, h=33) }
[Child 38704, Main Thread] WARNING: '!mSelection->IsValidIn(*mText)', file M:/src/widget/ContentCache.cpp:53
Assertion failure: IsValid(), at M:/src/widget/ContentCache.cpp:234

So, the value change by HTMLInputElement.stepUp() is not tracked...

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, I got it. This is a variation of bug 1835353. TextControlState already has mBoundFrame, but not yet recreated mTextEditor because of unsafe. Therefore, this path does not notify IMEContentObserver of the value change. A patch is coming.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1836806 - Make `TextControlState::SetValueWithoutTextEditor()` notify `IMEContentObserver` of value changes even if it has a bounding frame r=smaug!

This is an edge case which couldn't be fixed in bug 1835353.

In the testcase, TextControlState has new frame for the new
<input type="number"> when its stepUp() is called. However, its
TextEditor has not been recreated yet because it's not safe yet. Therefore,
SetValueWithoutTextEditor() is called, but I added the new path notifying
IMEContentObserver of the value changes does not run if mBoundFrame is not
nullptr. I don't remember why I did so (probably for avoiding performance
regressions as far as possible), but it does not make sense not to notify
IMEContentObserver if only mTextEditor has not been recreated because
IMEStateManager has not been reinitialized IMEContentObserver with new
anonymous <div>s yet because it requires new TextEditor instance.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1836806 - Make `TextControlState::SetValueWithoutTextEditor()` notify `IMEContentObserver` of value changes even if it has a bounding frame r=smaug!

This is an edge case which couldn't be fixed in bug 1835353.

In the testcase, TextControlState has new frame for the new
<input type="number"> when its stepUp() is called. However, its
TextEditor has not been recreated yet because it's not safe yet. Therefore,
SetValueWithoutTextEditor() is called, but I added the new path notifying
IMEContentObserver of the value changes does not run if mBoundFrame is not
nullptr. I don't remember why I did so (probably for avoiding performance
regressions as far as possible), but it does not make sense not to notify
IMEContentObserver if only mTextEditor has not been recreated because
IMEStateManager has not been reinitialized IMEContentObserver with new
anonymous <div>s yet because it requires new TextEditor instance.

Original Revision: https://phabricator.services.mozilla.com/D180767

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/478fa5088af6
Make `TextControlState::SetValueWithoutTextEditor()` notify `IMEContentObserver` of value changes even if it has a bounding frame r=smaug

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/40536 for changes under testing/web-platform/tests

Comment 11

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/478fa5088af6

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox115 to wontfix.

For more information, please visit BugBot documentation.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1825693

Comment 14

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20230614093740-b9e3497b939c.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9338777 [details]
Bug 1836806 - Make TextControlState::SetValueWithoutTextEditor() notify IMEContentObserver of value changes even if it has a bounding frame r=smaug!

Beta/Release Uplift Approval Request

• User impact if declined: This is a regression of bug 1825693, and this crash may cause loosing users' input data in the web app. The assertion detected a traditional bug which was not fixed in bug 1835353 which may cause some long standing crash bugs in the e10s world.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This applies the fix in bug 1835353 to another case. Therefore, this does not include new complicated logic etc.
• String changes made/needed:
• Is Android affected?: Yes

Comment 17

[Donal Meehan [:dmeehan]]

Comment on attachment 9338777 [details]
Bug 1836806 - Make TextControlState::SetValueWithoutTextEditor() notify IMEContentObserver of value changes even if it has a bounding frame r=smaug!

Approved for 115.0b6.

Comment 18

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eb3baa1099b7