Closed Bug 1549812 Opened 2 years ago Closed 2 years ago

crash in [@ mozilla::PresShell::ScrollFrameRectIntoView]


(Core :: Layout, defect)

Not set



Tracking Status
firefox-esr60 --- unaffected
firefox67 --- wontfix
firefox68 --- verified
firefox69 --- verified


(Reporter: tsmith, Assigned: emilio)


(Blocks 1 open bug)


(4 keywords, Whiteboard: [adv-main68-])


(3 files, 3 obsolete files)

Attached file testcase.html

This looks similar to bug 1535187 but it does not appear to be crashing on a framepoison address.

eip = 0xd28647d6   esp = 0xd86fc140   ebp = 0xd86fc278   ebx = 0xd7adede0
esi = 0xb62ba2c8   edi = 0xd86fc220   eax = 0xf56b77ff   ecx = 0xd871a200
edx = 0xd871f600   efl = 0x00210296
OS|Android|0.0.0 Linux 4.4.124+ #1 SMP PREEMPT Sun Nov 4 14:31:25 UTC 2018 i686
CPU|x86|GenuineIntel family 6 model 6 stepping 3|4
13|0||mozilla::PresShell::ScrollFrameRectIntoView(nsIFrame*, nsRect const&, mozilla::ScrollAxis, mozilla::ScrollAxis, mozilla::ScrollFlags)||3550|0x7
13|3||non-virtual thunk to nsListControlFrame::OnOptionSelected(int, bool)||0|0x2c
13|4||mozilla::dom::HTMLSelectElement::OnOptionSelected(nsISelectControlFrame*, int, bool, bool, bool)||669|0xf
13|5||mozilla::dom::HTMLSelectElement::SetOptionsSelectedByIndex(int, int, unsigned int)||800|0x25
13|7||mozilla::dom::HTMLOptionElement_Binding::set_selected(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLOptionElement*, JSJitSetterCallArgs)|s3:gecko-generated-sources:6465919b3fc3729e4b0d657d4a372e0d7357c8bc1cd26be69e5d04ede672887704c5e2ea7eefcf286c6f9fb3ea0b1d29f24c13187e7be10eaa73a0b0e72977ef/dom/bindings/HTMLOptionElementBinding.cpp:|425|0xe
13|8||bool mozilla::dom::binding_detail::GenericSetter<mozilla::dom::binding_detail::NormalThisPolicy>(JSContext*, unsigned int, JS::Value*)||3106|0x2b
13|9||CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&)||443|0x16
13|10||js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)||535|0xd
13|11||InternalCall(JSContext*, js::AnyInvokeArgs const&)||590|0x17
13|12||js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>)||744|0x37
13|13||SetExistingProperty(JSContext*, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyResult>, JS::ObjectOpResult&)||2879|0x44
13|14||bool js::NativeSetProperty<(js::QualifiedBool)1>(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&)||2908|0x31
13|15||Interpret(JSContext*, js::RunState&)||2847|0x123
13|16||js::RunScript(JSContext*, js::RunState&)||423|0x7
13|17||js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)||563|0xd
13|18||InternalCall(JSContext*, js::AnyInvokeArgs const&)||590|0x17
13|19||<name omitted>||606|0x7
13|20||JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>)||2647|0x51
13|21||mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&)|s3:gecko-generated-sources:07034a91c20d743b6b1cb0050fb45856e506111933106e79effdb8dcee60d394334ccec99923dca240d02a8a2423627e46882951c1689b39a2e7f0665bac7e9b/dom/bindings/EventHandlerBinding.cpp:|267|0x24
13|22||void mozilla::dom::EventHandlerNonNull::Call<nsCOMPtr<mozilla::dom::EventTarget> >(nsCOMPtr<mozilla::dom::EventTarget> const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*)|s3:gecko-generated-sources:400f5854eaa0ecaed3e4b6de4ef056ff5ed59f33939cee029fcff1b03eeff195f4c1df1e3e5019f96bbe28beb9ac8b7f3c48e72df24fb254eded6e83edd51671/dist/include/mozilla/dom/EventHandlerBinding.h:|363|0x34
13|24||mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*)||1045|0xc
13|25||mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool)||1240|0x1f
13|26||mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&)||349|0x18
13|27||mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&)||551|0x18
13|28||mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)||1047|0x23
13|30||nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult)||6641|0x17
13|31||nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult)||6441|0x16
13|32||non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult)||0|0x2e
13|33||nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult)||1313|0x25
13|34||nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult)||872|0x2e
13|36||nsDocLoader::OnStopRequest(nsIRequest*, nsresult)||598|0xf
13|37||non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsresult)||0|0x26
13|38||mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult)||568|0x21
13|41||nsThread::ProcessNextEvent(bool, bool*)||1180|0x16
13|42||NS_ProcessNextEvent(nsIThread*, bool)||486|0x11
13|49||XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&)||4686|0x8
13|50||XRE_main(int, char**, mozilla::BootstrapConfig const&)||4767|0xf
13|52||mozilla::BootstrapImpl::GeckoStart(_JNIEnv*, char**, int, mozilla::StaticXREAppData const&)||77|0x11
Flags: in-testsuite?
Flags: needinfo?(emilio)

Can't repro locally, any particular pref that's needed?

Flags: needinfo?(emilio) → needinfo?(twsmith)

Script to setup Android emulator sent

Flags: needinfo?(twsmith)

Yup, I'll try to set it up next week or such, apparently this is Android-specific.

Flags: needinfo?(emilio)

Where to even start. This is the caret code again. Diagnostics in a second.

Assignee: nobody → emilio
Flags: needinfo?(emilio)

Also I'm pretty sure this is frame poisoning.

ScrollToShowRect already considers that possibility, so not doing it on the
caller is a bug.

Ideally scroll observers shouldn't be able to run script, more to that in a

Attached file caret-events.txt

There are four caret events in there. The last one is the problematic one, that ends up running the ActionBarHandler.jsm code, and flushing layout via the Selection stringifier.

Instead, post the event for the next turn of the event loop.

In this case, what killed the frame is ActionBarHandler.jsm via

Depends on D31088

I think these should hold, everything that runs under them should just schedule
other stuff to some later date:

  • Synth mouse events -> scheduled as refresh driver observers.
  • Scroll events -> Scheduled as well.
  • Caret state change events -> Also scheduled after last patch.
  • IME and accessibility stuff -> I don't think they can reenter layout.

We can always revert this if it causes troubles, plus it shouldn't crash on
release so should be fine.

Depends on D31089

This one looks like fun too: bp-43903a6c-d68c-40ee-958b-c476e0190508

Yeah, that one's known, bug 1530190

Ting-Yu, do you know how can I run those tests locally? I think those tests just need to wait for a tick sometime now.

Flags: needinfo?(aethanyc)

A long time ago, I followed, and executed ./mach robocop testAccessibleCarets. I was able to run it in android emulator locally.

Today, I run it again. I hit bug 1552964 as well as this error.

Automation Error: No crash directory (/sdcard/tests/profile/minidumps) found on remote device
 0:26.85 ERROR runApp() exited with code 1
 0:27.31 INFO PROCESS-CRASH | Automation Error: Missing end of test marker (process crashed?)

I didn't dig deeper to tell whether it was my local issue or not. Maybe you'll have luck running it either in emulator or in real device.

Flags: needinfo?(aethanyc)

I'll land all patches but the first in a separate batch and separate bug for
regression tracking purposes.

Waiting on review of the test fix, this can land afterwards, though I'll probably land the three later patches in a separate bug.

Flags: needinfo?(emilio)
Blocks: 1553772

Comment on attachment 9064795 [details]
Bug 1549812 - Don't run arbitrary script from AccessibleCaretManager callbacks. r=TYLin

Revision D31089 was moved to bug 1553772. Setting attachment 9064795 [details] to obsolete.

Attachment #9064795 - Attachment is obsolete: true

Comment on attachment 9064796 [details]
Bug 1549812 - Try to assert a bit harder about stuff not flushing under our nose. r=TYLin,mats

Revision D31090 was moved to bug 1553772. Setting attachment 9064796 [details] to obsolete.

Attachment #9064796 - Attachment is obsolete: true

Comment on attachment 9066823 [details]
Bug 1549812 - fix testAccessibleCarets.js to account for more async event dispatching. r=TYLin

Revision D32194 was moved to bug 1553772. Setting attachment 9066823 [details] to obsolete.

Attachment #9066823 - Attachment is obsolete: true
Group: layout-core-security → core-security-release
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Want to request uplift to 68?

Flags: needinfo?(emilio)

Comment on attachment 9064793 [details]
Bug 1549812 - ScrollFrameRectIntoView should handle the frame going away. r=mats

Beta/Release Uplift Approval Request

  • User impact if declined: Potential crash on Android.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Open test-case on Fennec.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Just a missing check.
  • String changes made/needed: none
Flags: needinfo?(emilio)
Attachment #9064793 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9064793 [details]
Bug 1549812 - ScrollFrameRectIntoView should handle the frame going away. r=mats

android crash fix, approved for 68.0b5

Attachment #9064793 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified as fixed on the latest version of Nightly 68.0a1 (2019-05-26) using Samsung Galaxy Tab S3 (Android 8.0). I'll let the qe-verify flag till the verification on Beta, thanks.

Blocks: 1538758
QA Whiteboard: [qa-triaged]

Verified as fixed on Beta 68.0b5 using Samsung Galaxy Note 8 (Android 9). Due to my findings, I'll remove the qe-verify flag, thanks.

Flags: qe-verify+
QA Whiteboard: [qa-triaged] → [qa-triaged][adv-main68-]
QA Whiteboard: [qa-triaged][adv-main68-] → [qa-triaged]
Whiteboard: [adv-main68-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.