1677774 - Crash in [@ IPCError-browser | CommitFromIPC Invalid Transaction from Child - CanSet failed for field(s): TouchEventsOverrideIntern]

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Gabriele Svelto [:gsvelto]]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/f5747459-4531-408e-a210-254b00201116

Reason: EXC_BREAKPOINT / EXC_I386_BPT

Top 10 frames of crashing thread:

 0  libsystem_kernel.dylib __psynch_mutexwait
 1  libsystem_pthread.dylib _pthread_mutex_firstfit_lock_slow
 2  XUL google_breakpad::ExceptionHandler::WriteMinidump exception_handler.cc:302
 3  XUL google_breakpad::ExceptionHandler::WriteMinidump exception_handler.cc:317
 4  XUL CrashReporter::CreateMinidumpsAndPair nsExceptionHandler.cpp:3805
 5  XUL bool mozilla::ipc::CrashReporterHost::GenerateMinidumpAndPair CrashReporterHost.h:71
 6  XUL mozilla::dom::ContentParent::GeneratePairedMinidump ContentParent.cpp:3830
 7  XUL mozilla::dom::ContentParent::KillHard ContentParent.cpp:3865
 8  XUL mozilla::ipc::IPCResult::Fail ProtocolUtils.cpp:61
 9  XUL mozilla::dom::syncedcontext::Transaction SyncedContextInlines.h:92

Not all the crashes under this signature have fission enabled but some do. I will attach the actual stack trace shortly.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: Stack traces of the main process of crash f5747459-4531-408e-a210-254b00201116

Comment 2

[Andrew McCreight [:mccr8]]

Steven, it looks like you moved this field to the BC. Maybe you could take a look at this crash? Thanks.

Comment 3

[Steven MacLeod [:smacleod]]

(In reply to Andrew McCreight [:mccr8] from comment #2)

Steven, it looks like you moved this field to the BC. Maybe you could take a look at this crash? Thanks.

Sure thing, thanks for calling this out.

Taking another look at the code involved here, I have a theory for what's happening:

When BrowsingContext::SetTouchEventsOverride is called, we walk the context's descendants and set their TouchEventsOverrideinternal to TouchEventsOverride::None (if it isn't already). I must have forgotten about this when writing the BrowsingContext::CanSet for TouchEventsOverrideInternal and made it CheckOnlyOwningProcessCanSet. If any of these descendants are OOP and have an override set this crash will be triggered.

I can fix this by either:

1. Allowing cross-origin sets (so if a content process is pwned, it could change the TouchEventsOverrideInternal for cross origin documents)
2. Change the semantics of setting TouchEventsOverrideInternal to not match the old pre-fission behaviour in one of the following ways:
   A. Don't inherit overrides from ancestor contexts at all.
   B. Inherit from ancestor contexts, but don't clear any descendent overrides when an ancestor's override changes.

:whimboo, I think you'd be in a much better position to make the call (or point me at who should) about which path to take ^.

Comment 4

[Steven MacLeod [:smacleod]]

(In reply to Steven MacLeod [:smacleod] from comment #3)

:whimboo, I think you'd be in a much better position to make the call (or point me at who should) about which path to take ^.

Woops, confused myself because you wrote a related patch in https://phabricator.services.mozilla.com/D96705.

jdescottes, you're who I'm looking for. Please see Comment 3.

Comment 5

[Julian Descottes [:jdescottes]]

Sorry I forgot to reply here!

In my opinion this is not something we would want to expose to a DevTools (or test automation) end user. I don't think users would benefit from this. Enabling touch simulation should remain a global feature.

If we go for 2.A then DevTools would have to emulate the inheritance somehow.
I think 2.B can be a good compromise. We will only ever set overrides on the topmost context. So as long as updating the override is properly inherited by descendent contexts which didn't set a specific override this would be ok.

Would the following scenario work with 2.B?

• assuming two contexts, parent and child
• both have initially touchEventsOverride set to "none" (default)
• start touch simulation, which sets parent.touchEventsOverride = "enabled"
• child should now inherit from this and be "enabled" as well
• stop touch simulation, which sets parent.touchEventsOverride = "none"
• child should also revert back to "none"

If this works with 2.B, I think we should be ok. Adding :micah, :bwerth and :rcaliman in cc.

Comment 6

[Steven MacLeod [:smacleod]]

(In reply to Julian Descottes [:jdescottes] from comment #5)

If this works with 2.B, I think we should be ok.

Yup, that scenario will work. The difference would only be noticed in the following scenario:

1. assuming two contexts, parent and child.
2. both initially have touchEventsOVerride set to "none" (default)
3. child.touchEventsOverride = "enabled"
4. parent.touchEventsOverride = "disabled"

The old behaviour would result in both parent and child being effectively "disabled" and child.touchEventsOverride == "none" . Going with 2.B, child would remain "enabled" and not inherit from the parent because it had previously been set directly.

If touchEventsOverride is only ever set directly on the top context, 2.B shouldn't result in any differences.

Comment 7

[Julian Descottes [:jdescottes]]

Great, I think we are good with 2.B from a DevTools & test automation standpoint

Comment 8

[Steven MacLeod [:smacleod]]

Attachment: Bug 1677774 - don't clear descendant's overrides when setting TouchEventsOverride. r?farre!,jdescottes

This should prevent content process crashes when there are
cross-origin descendants with an override set.

There is a slight change in behaviour when an override is set after
a descendant context has previously set one. This shouldn't be a
problem since devtools only wants to expose touch simulation at the
tab level. Additionally this new behaviour is reasonable and would
allow devtools to implement the same semantics by doing the clearing
themselves (if needed).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea3b56f48dfd
don't clear descendant's overrides when setting TouchEventsOverride. r=farre

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ea3b56f48dfd

Comment 11

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

There is one crash with this signature for the first Nightly which contains the commit from this bug: bp-1f565991-e715-488d-ab7e-a07a20201201
Please take a look if anything is left to be done.

Comment 12

[Neha Kochar [:neha]]

smacleod said: "Theory is that responsive design mode triggers this more often than you’d expect because by design it causes a navigation at the same time as changing touch events simulation."
Steven is working on a fix to address this theory.

Comment 13

[Chris Peterson [:cpeterson]]

Nika says, IIUC, all uses of BrowsingContext::CheckOnlyOwningProcessCanSet are buggy.

I filed bug 1688722 to replace uses of BrowsingContext::CheckOnlyOwningProcessCanSet. (BrowsingContext::CheckOnlyOwningProcessCanSet was just renamed to LegacyCheckOnlyOwningProcessCanSet in bug 1688715.)

Comment 14

[Steven MacLeod [:smacleod]]

Attachment: Bug 1677774 - Relax CanSet for TouchEventsOverrideInternal. r=nika!

As a temporary fix to prevent crashes we are loosening the CanSet
restrictions for TouchEventsOverrideInternal. In the future this will
be fixed properly by changing DevTools to set this property from the
parent only, at which time tighter restrictions on setting
TouchEventsOverrideInternal may be added.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6969e6d61e8b
Relax CanSet for TouchEventsOverrideInternal. r=nika

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/6969e6d61e8b

Comment 17

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

Change the status for beta to have the same as nightly and release.
For more information, please visit auto_nag documentation.