1144759 - FilePickerParent::FileSizeAndDateRunnable::~FileSizeAndDateRunnable() releases main thread objects off the main thread

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Andrew McCreight [:mccr8]]

I see a fair number of crashes like this on Nightly:

https://crash-stats.mozilla.com/report/index/e13e8124-1e55-4560-b40c-e13e92150317#allthreads

I'm not sure how bad of a security problem this is given that we're always crashing but maybe the code is otherwise trying to use these objects.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

FileSizeAndDateRunnable (and anything that jumps around several threads) should be holding FileImpl, not nsIDOMFile.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

I guess that's because FileImpl has threadsafe AddRef and Release?

Comment 3

[Andrew McCreight [:mccr8]]

This looks like a regression from bug 910384.  How bad is this security-wise?

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8579618 [details] [diff] [review]
patch

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

Yeah, FileImpl is the threadsafe (and potentially shared) piece for blobs. Blob/File are the DOM parts, and they're CC'd, etc.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This is e10s-only, so it only affects Nightly.

Comment 6

[Andrew McCreight [:mccr8]]

Ah, great.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8579618 [details] [diff] [review]
patch

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

::: dom/ipc/FilePickerParent.h
@@ +7,4 @@
>  #ifndef mozilla_dom_FilePickerParent_h
>  #define mozilla_dom_FilePickerParent_h
>  
>  #include "nsIDOMFile.h"

Oh, you might be able to lose this now.

Comment 8

[Andrew McCreight [:mccr8]]

I'm going to mark this as moderate, because it seems like it would be pretty hard to exploit.  Note that we're hitting a release-mode assert here, so somehow you'd have to get this runnable to run some code before any CC-ed object is addrefed or released, plus whatever trickiness there is to exploit any race condition.  It would still be nice to get this fixed so we can find other threading issued without digging through all of the crashes for this signature.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/565712db304c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/565712db304c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Does this affect B2G at all?

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

No, I believe they have their own separate file picker.