1170084 - AccessibleCaretManager: Assertion failure: nsContentUtils::IsSafeToRunScript()

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Steps to reproduce:
1. Enable "layout.accessiblecaret.enabled"
2. Open https://en.m.wikipedia.org/wiki/Firefox
3. Select some text to make AccessibleCaret shown.
4. Click the image "A screenshot of Firefox 33.0.3 running within the Xfce desktop environment"

Actual result:
The tab crashes with full callstack dump here. https://pastebin.mozilla.org/8835292

Expected result:
The tab does not crash.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Hi Olli,

Could you explain further why it's needed to add MOZ_ASSERT(nsContentUtils::IsSafeToRunScript()) in [1] as you suggested in bug 1155493 comment 9? We now hit this assert. Thanks.

https://hg.mozilla.org/mozilla-central/file/9eae3880b132/layout/base/AccessibleCaretManager.cpp#l892

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

If you're dispatching an event, the expectation is that some listener may handle it, and
that listener is possibly implemented in JS. However if it is not safe to run scripts, we shouldn't be in a state where we try to run scripts (the event listener).

So, something on the stack has explicitly used nsAutoScriptBlocker or similar to indicate that it is
very much unsafe to run any scripts (running scripts when it it not safe to run them may cause crashes, security issues and what not).

So, check where the script blocker is used and why.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Looks like the script blocker comes from 
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=38a6cc715880#4544

Would it be possible to dispatch mozcaretstatechanged asynchronously using a script runner?
That would mean the event is dispatched when it is safe to dispatch it - when the last script blocker is removed from stack.
Replace doc->DispatchEvent(event, ...) with something like
(new AsyncEventDispatcher(doc, event))->RunDOMEventWhenSafe();

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Olli, 

Thank you very much for explaining that thoroughly. I think we could use AsyncEventDispatcher for dispatching the event in this case.

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: MozReview Request: Bug 1170084 - Dispatch CaretStateChangedEvent via AsyncEventDispatcher. r?mtseng

Bug 1170084 - Dispatch CaretStateChangedEvent via AsyncEventDispatcher. r?mtseng

We should not dispatch an event if it is not safe to run script since
the event handlers might be implemented by Javascript.
To fix this, we always use AsyncEventDispatcher to dispatch the event.

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8615848 [details]
MozReview Request: Bug 1170084 - Dispatch CaretStateChangedEvent via AsyncEventDispatcher. r?mtseng

Bug 1170084 - Dispatch CaretStateChangedEvent via AsyncEventDispatcher. r?mtseng

We should not dispatch an event if it is not safe to run script since
the event handlers might be implemented by Javascript.
To fix this, we always use AsyncEventDispatcher to dispatch the event.

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8615848 [details]
MozReview Request: Bug 1170084 - Dispatch CaretStateChangedEvent via AsyncEventDispatcher. r?mtseng

https://reviewboard.mozilla.org/r/10371/#review9081

LGTM

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f10a253891dc

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/795725455d95

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/795725455d95