"Hit MOZ_CRASH(nsStructuredCloneContainer not thread-safe)" with Notification from Worker

RESOLVED FIXED in Firefox 41, Firefox OS master

Status

()

Core
DOM: Workers
--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: nsm)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla42
assertion, csectype-race, sec-high, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox40 unaffected, firefox41+ fixed, firefox42 fixed, firefox-esr38 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-v2.2r unaffected, b2g-master fixed)

Details

(Whiteboard: [post-critsmash-triage][b2g-adv-main2.5-])

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8633841 [details]
testcase

In a debug build:

Hit MOZ_CRASH(nsStructuredCloneContainer not thread-safe) at dom/base/nsStructuredCloneContainer.cpp:25
(Reporter)

Comment 1

3 years ago
Created attachment 8633842 [details]
stack
Keywords: csectype-race, sec-high
Created attachment 8641295 [details] [diff] [review]
Fix Notification.data structured cloning on workers.

Andrew, could you check the CC and Hold/DropJSObjects/ExposeActive for mData? This code runs on main thread and workers.

Robert for making sure I haven't broken any semantics.
Attachment #8641295 - Flags: review?(continuation)
Comment on attachment 8641295 [details] [diff] [review]
Fix Notification.data structured cloning on workers.

Review of attachment 8641295 [details] [diff] [review]:
-----------------------------------------------------------------

The CC stuff looks good to me.
Attachment #8641295 - Flags: review?(continuation) → review+
status-firefox41: --- → affected

Comment 4

3 years ago
Comment on attachment 8641295 [details] [diff] [review]
Fix Notification.data structured cloning on workers.

Review of attachment 8641295 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Nikhil! lgtm, if the tests are green we're good to go.
Attachment #8641295 - Flags: review?(robertbindar) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/02652b4367625e2771366b18bf2e95753c240e26
changeset:  02652b4367625e2771366b18bf2e95753c240e26
user:       Nikhil Marathe <nsm.nikhil@gmail.com>
date:       Thu Jul 30 12:44:14 2015 -0700
description:
Bug 1183954 - Fix Notification.data structured cloning on workers. r=robertbindar,mccr8

Rather than store a non-thread-safe refcounted nsIStructuredCloneContainer, store the base64 representation.
Caches a jsval the first time an attempt to access data is made from content script.
Comment on attachment 8641295 [details] [diff] [review]
Fix Notification.data structured cloning on workers.

Approval Request Comment
[Feature/regressing bug #]: Bug 916893
[User impact if declined]: Crash when creating a Notification on worker with the data field set.
[Describe test coverage new/current, TreeHerder]: Added structured clone tests in dom/workers/
[Risks and why]: No risk to taking this patch.
[String/UUID change made/needed]: None.
Attachment #8641295 - Flags: approval-mozilla-aurora?
Attachment #8641295 - Flags: approval-mozilla-aurora?
Comment on attachment 8642090 [details] [diff] [review]
Patch for aurora.

Approval Request Comment
[Feature/regressing bug #]: Bug 916893
[User impact if declined]: Crash when creating a Notification on worker with the data field set.
[Describe test coverage new/current, TreeHerder]: Added structured clone tests in dom/workers/
[Risks and why]: No risk to taking this patch.
[String/UUID change made/needed]: None.
Attachment #8642090 - Flags: approval-mozilla-aurora?
And because of 'auto' hiding raw pointer usage we nicely leak here.
https://treeherder.mozilla.org/logviewer.html#?job_id=12395451&repo=mozilla-inbound
(In reply to Olli Pettay [:smaug] from comment #11)
> And because of 'auto' hiding raw pointer usage we nicely leak here.
> https://treeherder.mozilla.org/logviewer.html#?job_id=12395451&repo=mozilla-
> inbound

Yeah, that's bad.  Wonder if we can use static analysis to catch these kinds of mistakes.
FYI, I'm about to land a fix for the leak.
Comment on attachment 8642151 [details] [diff] [review]
don't leak

Review of attachment 8642151 [details] [diff] [review]:
-----------------------------------------------------------------

Not that it matters, but r=me.
Attachment #8642151 - Flags: review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12)
> (In reply to Olli Pettay [:smaug] from comment #11)
> > And because of 'auto' hiding raw pointer usage we nicely leak here.
> > https://treeherder.mozilla.org/logviewer.html#?job_id=12395451&repo=mozilla-
> > inbound
> 
> Yeah, that's bad.  Wonder if we can use static analysis to catch these kinds
> of mistakes.

Perhaps provide a MakeRefPtr<> like MakeUnique<> and then have an analyzer enforce that for pointer types?
https://hg.mozilla.org/mozilla-central/rev/02652b436762
https://hg.mozilla.org/mozilla-central/rev/7921dfab7eb3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: --- → fixed
status-firefox42: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Created attachment 8642484 [details] [diff] [review]
Patch for beta.

Updated to include leak fixes. Please see previous approval request.
Attachment #8642090 - Attachment is obsolete: true
Attachment #8642090 - Flags: approval-mozilla-aurora?
Attachment #8642484 - Flags: approval-mozilla-aurora?
Comment on attachment 8642484 [details] [diff] [review]
Patch for beta.

I think you meant beta as aurora is 42 (and fixed).
Attachment #8642484 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
(In reply to Sylvestre Ledru [:sylvestre] from comment #19)
> Comment on attachment 8642484 [details] [diff] [review]
> Patch for aurora.
> 
> I think you meant beta as aurora is 42 (and fixed).

Well, I asked for approval before the branch. The previous comment still applies for beta approval.
Comment on attachment 8642484 [details] [diff] [review]
Patch for beta.

This really needs to  be uplifted. This patch should apply to beta too
Attachment #8642484 - Attachment description: Patch for aurora. → Patch for beta.
Comment on attachment 8642484 [details] [diff] [review]
Patch for beta.

This patch has been in Nightly for over 2 weeks. Seems safe to uplift to Beta41.
Attachment #8642484 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Tracked since this is a security bug.
tracking-firefox41: --- → +
When I reviewed the patch I noticed that it does have a UUID change. Jorge, in the past Sylvestre asked me to check with you on the UUID changes. Do you see a concern here?
Flags: needinfo?(jorge)
Looks okay to me.
Flags: needinfo?(jorge)
https://hg.mozilla.org/releases/mozilla-beta/rev/d0f16a9c00a0
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-v2.2r: --- → unaffected
status-firefox40: --- → unaffected
status-firefox41: affected → fixed
status-firefox-esr38: --- → unaffected
Whiteboard: [post-critsmash-triage]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][b2g-adv-main2.5-]
You need to log in before you can comment on or make changes to this bug.