878703 - Cleanup usage of IO thread only objects

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jan Varga [:janv]]

Attachment: patch v0.1

mFileManagerInfos member in IndexedDatabaseManager supposed to be touched only on the IO thread, unfortunately it is not.
This patch fixes it and also adds AssertIsOnIOThread() to all IO thread only methods to make the thread they are running on more obvious.

Temp storage implementation depends on this fix.

Comment 1

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

Comment on attachment 757241 [details] [diff] [review]
patch v0.1

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

Looks good! r=me with just a few changes:

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +644,5 @@
>  
>    return NS_OK;
>  }
> +
> +GetFileReferencesHelper::GetFileReferencesHelper(const nsACString& aOrigin,

Nit: Just inline this

@@ +659,5 @@
> +{
> +}
> +
> +nsresult
> +GetFileReferencesHelper::DispatchAndReturnFileReferences(int32_t* aMemRefCnt,

Assert that you start out on the main thread here.

::: dom/indexedDB/IndexedDatabaseManager.h
@@ +96,5 @@
>    AsyncDeleteFile(FileManager* aFileManager,
>                    int64_t aFileId);
>  
> +  nsresult
> +  GetFileReferences(const nsACString& aOrigin,

Nit: Make this 'BlockAndGetFileReferences', and add a big scary comment saying that this is to be used in testing-only code because it blocks the main thread.

Comment 2

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23a34163cd70

Comment 3

[Jan Varga [:janv]]

Attachment: interdiff

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23a34163cd70

Comment 5

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

This is a thread-unsafe access of a hash table. s-s for now until someone from the sec team can rate it.

Comment 6

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

Comment on attachment 757241 [details] [diff] [review]
patch v0.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unclear
User impact if declined: Crashes, maybe an exploit. Depends on a race so it might be tough.
Testing completed (on m-c, etc.): It's been on m-c for a while now.
Risk to taking this patch (and alternatives if risky): No risks that we're aware of.
String or IDL/UUID changes made by this patch: None

Comment 7

[Alex Keybl [:akeybl]]

(In reply to ben turner [:bent] from comment #6)
> User impact if declined: Crashes, maybe an exploit. Depends on a race so it
> might be tough.

Can we get more confidence than maybe an exploit? If so, can we make this a sec-critical.

> Risk to taking this patch (and alternatives if risky): No risks that we're
> aware of.

Low enough risk for the final beta of a release?

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d2f19cdab655

Comment 9

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

(In reply to Alex Keybl [:akeybl] from comment #7)
> Can we get more confidence than maybe an exploit? If so, can we make this a
> sec-critical.

We'd need someone from the sec team to weigh in on this.

> Low enough risk for the final beta of a release?

I'm not sure, so I guess not :(

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Did this affect 21 or earlier, such as ESR17?

Also, before this went in initially, it should have gone through sec-approval and gotten a rating, per https://wiki.mozilla.org/Security/Bug_Approval_Process.
  
I'm not sure of the rating here. Asking Dan.

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 757241 [details] [diff] [review]
patch v0.1

We're not confident enough in the fix (even if this is sec-crit) to take in a final beta.