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

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Jesse Ruderman]

Attachment: testcase

In a debug build:

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

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: 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.

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Robert Bindar]

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.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/02652b4367625e2771366b18bf2e95753c240e26
changeset:  02652b4367625e2771366b18bf2e95753c240e26
user:       Nikhil Marathe <[e-mail]>
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 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=263a7ed9dc0e

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch for aurora.

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 11

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

And because of 'auto' hiding raw pointer usage we nicely leak here.
https://treeherder.mozilla.org/logviewer.html#?job_id=12395451&repo=mozilla-inbound

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 13

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

FYI, I'm about to land a fix for the leak.

Comment 14

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

Attachment: don't leak

https://hg.mozilla.org/integration/mozilla-inbound/rev/7921dfab7eb3

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8642151 [details] [diff] [review]
don't leak

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

Not that it matters, but r=me.

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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?

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/02652b436762
https://hg.mozilla.org/mozilla-central/rev/7921dfab7eb3

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch for beta.

Updated to include leak fixes. Please see previous approval request.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8642484 [details] [diff] [review]
Patch for beta.

I think you meant beta as aurora is 42 (and fixed).

Comment 20

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(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 21

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8642484 [details] [diff] [review]
Patch for beta.

This really needs to  be uplifted. This patch should apply to beta too

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked since this is a security bug.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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?

Comment 25

[Jorge Villalobos [:jorgev] (he/him)]

Looks okay to me.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d0f16a9c00a0