1396323 - Intermittent browser_bug1025195_switchToTabHavingURI_aOpenParams.js | Test timed out | application crashed [@ mozilla::dom::DocGroup::ValidateAccess] (Assertion failure: IsSafeToRun(), at SchedulerGroup.h:74) when Gecko 57 merges to Beta on 2017-09-20

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect, P5)

Description

[Treeherder Bug Filer]

Filed by: rvandermeulen [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=128041886&repo=try

https://queue.taskcluster.net/v1/task/KWUcw8rxQlKeM8-WLz59hg/runs/0/artifacts/public/logs/live_backing.log

Bisection on Try has confirmed that this was caused by bug 1376525. Appears to only affect Linux, but always in the same test. Failure rate is ~50%.

The patch below should be enough to reproduce.
https://hg.mozilla.org/try/rev/70cb6436d870bc25344b98e8ec3ed1be9cf9afc6

Comment 1

[Intermittent Failures Robot]

8 failures in 939 pushes (0.009 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* try: 8

Platform breakdown:
* linux32: 3
* linux64-stylo: 2
* linux64: 2
* linux32-stylo: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1396323&startday=2017-08-28&endday=2017-09-03&tree=all

Comment 2

[Ryan Hunt [:rhunt]]

My guess is:

1. A script is long running with DocGroup for tab A active
2. A tab switch happens
3. JS interrupt runs, triggering new tab B to paint
4. Keyboard APZ creates a focus target for this paint
5. Focus target collects event targets that would be hit for a key event (using an eVoid event)
6. One of them is an <input> which lazily initializes [1]
7. The lazy initialization causes a DOM modification for tab B [2] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/layout/forms/nsTextControlFrame.cpp#1202

The assertion is because the DocGroup for tab A is active, while we are modifying B.

I don't think we need to lazy init <input> when we are dispatching eVoid events. Fixing that resolved this issue for me. I can't see any other GetEventTargetParent implementations that do similar, so I think that's all that needed.

[1] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/dom/html/HTMLInputElement.cpp#3611

Comment 3

[Ryan Hunt [:rhunt]]

Attachment: Bug 1396323 - Don't initialize HTMLInputElement editor for eVoidEvent.

Keyboard APZ dispatches an eVoidEvent to gather all event targets that a key event
would normally go to. This can sometimes trigger an HTMLInputElement to initialize its
editor, which can cause unnecessary DOM modifications.

Review commit: https://reviewboard.mozilla.org/r/176500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176500/

Comment 4

[(Out Sep 17-23) Ryan VanderMeulen]

Comment on attachment 8904716 [details]
Bug 1396323 - Don't initialize HTMLInputElement editor for eVoidEvent.

Looks good on Try!

Comment 5

[(no longer active)]

Comment on attachment 8904716 [details]
Bug 1396323 - Don't initialize HTMLInputElement editor for eVoidEvent.

https://reviewboard.mozilla.org/r/176500/#review181760

::: dom/html/HTMLInputElement.cpp:3582
(Diff revision 1)
>        aVisitor.mEvent->mClass == eMutationEventClass) {
>      return false;
>    }
>  
>    switch (aVisitor.mEvent->mMessage) {
> +  case eVoidEvent:

I think it'd be nice to include the description in the commit message as a code comment around here instead.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3067ce95b439
Don't initialize HTMLInputElement editor for eVoidEvent. r=ehsan

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3067ce95b439