Crash in mozilla::dom::PostMessageEvent::Run

RESOLVED FIXED in Firefox 56

Status

()

--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: philipp, Assigned: tnguyen)

Tracking

({crash, regression})

55 Branch
mozilla56
crash, regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 wontfix, firefox56 fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
Andrea, looks like its triggering a diagnostic assertion you last touched.
Flags: needinfo?(amarchesini)
I saw this while trying to use the back button in a private browsing session.
This happened again, same usage scenario: keep pressing back in a private browsing session and eventually you crash.
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 ...
Hi Thomas, could you help to look into this bug?
Flags: needinfo?(tnguyen)
(Assignee)

Updated

a year ago
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Flags: needinfo?(tnguyen)
Comment hidden (obsolete)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
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

a year ago
mozreview-review
Comment on attachment 8887350 [details]
Bug 1373735 - Skip checking mPrivateBrowsingId in case system principal

https://reviewboard.mozilla.org/r/158180/#review163510
Attachment #8887350 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 11

a year ago
(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

a year ago
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...
Flags: needinfo?(tnguyen)
(Assignee)

Comment 13

a year ago
(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
Flags: needinfo?(tnguyen)

Comment 14

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3351fac5174b
Skip checking mPrivateBrowsingId in case system principal r=baku
Keywords: checkin-needed

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3351fac5174b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Please request Beta approval on this when you get a chance.
Flags: needinfo?(tnguyen)
(Assignee)

Comment 17

a year ago
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
Flags: needinfo?(tnguyen)
Attachment #8887350 - Flags: approval-mozilla-beta?
Thanks Thomas and baku for the patch and review!
Flags: needinfo?(amarchesini)
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.
Attachment #8887350 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox55: affected → wontfix
You need to log in before you can comment on or make changes to this bug.