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

Status: RESOLVED WORKSFORME

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

Description

[Gabriele Svelto [:gsvelto]]

This bug is for crash report bp-ea87b940-72e6-4286-9611-15c8c0200614.

Top 10 frames of crashing thread:

0 ntdll.dll NtWaitForAlertByThreadId 
1 ntdll.dll RtlSleepConditionVariableSRW 
2 kernelbase.dll SleepConditionVariableSRW 
3 mozglue.dll mozilla::detail::ConditionVariableImpl::wait mozglue/misc/ConditionVariable_windows.cpp:50
4 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1152
5 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:501
6 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:109
7 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308
8 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:290
9 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:137

I found a few of those crashes in nightly, but there's more going back at least two more versions so I'm not sure if it's a regression. I'll attach the stack trace from the main process shortly.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: ea87b940-72e6-4286-9611-15c8c0200614-upload_file_minidump_browser.log

This is the full stack trace of the main process for this crash.

Comment 2

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

How should we deal with "Invalid Transaction from Child" errors?

Comment 3

[Andreas Farre [:farre]]

tl;dr The way to deal with "Invalid Transaction from Child" is to crash the process that initiated the transaction. The question is rather, how do we avoid invalid transactions to begin with.

This happens because CanSet on the particular field fails. The idea here is that we should never be the cause of CanSet returning false, instead it should always mean that the ContentChild that tried to modify the field has been compromised and should be taken down.

This particular failure is BrowsingContext::CanSet(FieldIndex<IDX_AllowPlugins>, ...) which uses the common implementation in CheckOnlyOwningProcessCanSet which does several checks, but the failing one in this case is that the BrowsingContext that we try to modify AllowPlugins on is nither not controlled by the ContentChild that did the modification or the ContentChild which we're in the process of changing remoteness to.

This means that either we or "someone else" is trying to set AllowPlugins, or there is a bug in BrowsingContext transactions. For the former we should crash if "someone else" is doing it and not do it ourselves, but both should actually crash this way. If there's a bug in BrowsingContext transactions it's more difficult. My gut feeling is that this shouldn't happen, but I don't really trust my gut feeling. It might be that we've missed an edge case here. I wonder how CanSet works with discarded contexts.

Nika added the failure formatting that allows us to see which fields that we're not handling correctly, which means we can search for it on
crash-stats.

I've looked at the cases when we call BrowsingContext::SetAllowPlugins, and they all seem to do it when there's an nsDocShell present, but that makes me wonder if we're keeping an nsDocShell alive after a remoteness change somewhere (or a tab close for that matter).

Maybe we should add a warning when setting a field on a detached browsing context. That would at least tell us if that's what we're doing.

Sorry for answering with more questions :/

Comment 4

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

Closing because no crashes reported for 12 weeks.