1373735 - Crash in mozilla::dom::PostMessageEvent::Run

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-35dc1568-1a25-41d9-90f9-146d30170616.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::dom::PostMessageEvent::Run() 	dom/base/PostMessageEvent.cpp:112
1 	xul.dll 	mozilla::SchedulerGroup::Runnable::Run() 	xpcom/threads/SchedulerGroup.cpp:359
2 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1321
3 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:96
4 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:301
5 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
6 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
7 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp:156
8 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:271
9 	xul.dll 	XRE_RunAppShell() 	toolkit/xre/nsEmbedFunctions.cpp:893
10 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:269
11 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
12 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
13 	xul.dll 	XRE_InitChildProcess(int, char** const, XREChildData const*) 	toolkit/xre/nsEmbedFunctions.cpp:709
14 	xul.dll 	mozilla::BootstrapImpl::XRE_InitChildProcess(int, char** const, XREChildData const*) 	toolkit/xre/Bootstrap.cpp:65
15 	firefox.exe 	content_process_main(mozilla::Bootstrap*, int, char** const) 	ipc/contentproc/plugin-container.cpp:64
16 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:115
17 	firefox.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253
18 	kernel32.dll 	BaseThreadInitThunk 	
19 	ntdll.dll 	__RtlUserThreadStart 	
20 	ntdll.dll 	_RtlUserThreadStart

reports with this signature are regressing in volume since firefox 55. there was an attempted fix in bug 1345961 - crashes are now happening in MOZ_RELEASE_ASSERT(ChromeUtils::IsOriginAttributesEqualIgnoringFPD(mProvidedPrincipal->OriginAttributesRef(), targetPrin->OriginAttributesRef())) (Unexpected postMessage call to a window with mismatched origin attributes) that got added there.

this is currently the #25 browser crash in early data from 55.0b.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Andrea, looks like its triggering a diagnostic assertion you last touched.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

I saw this while trying to use the back button in a private browsing session.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

This happened again, same usage scenario: keep pressing back in a private browsing session and eventually you crash.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hey :baku, have you got a chance to take a look at this?

Note I didn't get a crash on Nightly Win10 or on Mac when I tried comment 3 ...

Comment 5

[Ethan Tseng [:ethan]]

Hi Thomas, could you help to look into this bug?

Comment 6

[Thomas Nguyen (:tnguyen)]

At the first see FF crash when targetPrin is SystemPrincipal (which has mPrivateBrowsingId == 0). It's triggering the diagnostic assertion with contentprincipal mPrivateBrowsingId == 1

Comment 7

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1373735 - Skip checking mPrivateBrowsingId in case system principal

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

Comment 8

[Thomas Nguyen (:tnguyen)]

Baku, could you please take a look at this?
I could not reproduce the issue but from crash signature, I only see it happened when posting message between systemPrincipal and contentPrincipal.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8887350 [details]
Bug 1373735 - Skip checking mPrivateBrowsingId in case system principal

https://reviewboard.mozilla.org/r/158180/#review163510

Comment 10

[Thomas Nguyen (:tnguyen)]

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

Comment 11

[Thomas Nguyen (:tnguyen)]

(In reply to Andrea Marchesini [:baku] from comment #9)
> Comment on attachment 8887350 [details]
> Bug 1373735 - Skip checking mPrivateBrowsingId in case system principal
> 
> https://reviewboard.mozilla.org/r/158180/#review163510

Thanks for your review.

Comment 12

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Can I ask why we were triggering this with the system principal? How would a content principal get a reference to a system principal window with which to postMessage? That in itself seems concerning...

Comment 13

[Thomas Nguyen (:tnguyen)]

(In reply to Tom Ritter [:tjr] from comment #12)
> Can I ask why we were triggering this with the system principal? How would a
> content principal get a reference to a system principal window with which to
> postMessage? That in itself seems concerning...

Hmm, that's an interesting point, from the crash report, I could not see how we ended up with that.
But I guess that's possible, for example, if we did get content principal from system principal like
http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/dom/base/nsGlobalWindow.cpp#9213

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3351fac5174b
Skip checking mPrivateBrowsingId in case system principal r=baku

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3351fac5174b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 17

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8887350 [details]
Bug 1373735 - Skip checking mPrivateBrowsingId in case system principal

Approval Request Comment
[Feature/Bug causing the regression]: 1345961
[User impact if declined]: crash, particularly when navigating in PBM, comment 3
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Comment 3, probably 
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: No
[Why is the change risky/not risky?]: Adding a condition check to prevent MOZ_DIAGNOSTIC_ASSERT
[String changes made/needed]: No

Comment 18

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thanks Thomas and baku for the patch and review!

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8887350 [details]
Bug 1373735 - Skip checking mPrivateBrowsingId in case system principal

diagnostic asserts are only enabled in nightly and dev edition, I'm going to let this ride 56.