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

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

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.

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.

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.

(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

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

Comment on attachment 8642484 [details] [diff] [review] Patch for beta. I think you meant beta as aurora is 42 (and fixed).

(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

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.

Tracked since this is a security bug.

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?

Looks okay to me.