Bug 73260 – IndexedDB: make IDBBackingStore ThreadSafeRefCounted instead of RefCounted

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[David Grogan]

make IDBBackingStore ThreadSafeRefCounted instead of RefCounted

[David Grogan]

Created attachment 116840 [details]
Patch

[David Grogan]

Tony, could you review this?

[David Levin]

Comment on attachment 116840 [details]
Patch

This isn't safe to do based on my cursory search of what derives from this. (Note if something is ThreadSafeRefCounted then all container objection should like be ThreadSafeRefCounted.)

[David Grogan]

(In reply to comment #3)
> (From update of attachment 116840 [details])
> This isn't safe to do based on my cursory search of what derives from this.

IDBLevelDBBackingStore is the only thing that derives from this.  Could you tell me what in IDBLevelDBBackingStore makes this unsafe?

> (Note if something is ThreadSafeRefCounted then all container objection should like be ThreadSafeRefCounted.)

I'm parsing this as "container objects should likely be."  I think you mean that the member variables of IDBLevelDBBackingStore should also be ThreadSafeRefCounted.  Is that right?

[David Levin]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 116840 [details] [details])
> > This isn't safe to do based on my cursory search of what derives from this.
> 
> IDBLevelDBBackingStore is the only thing that derives from this.  Could you tell me what in IDBLevelDBBackingStore makes this unsafe?

I didn't know there was only one. I only found one quick though.

> 
> > (Note if something is ThreadSafeRefCounted then all container objection should like be ThreadSafeRefCounted.)
> 
> I'm parsing this as "contained objects should likely be."

:Nice autocorrection! I was multitasking a real time im conversation with this and not doing a great job :(.



> I think you mean that the member variables of IDBLevelDBBackingStore should also be ThreadSafeRefCounted.  Is that right?

Yep. For example, IDBLevelDBBackingStore contains "String m_identifier;" Now if that string has been shared with any other data structure on any other thread, then it isn't safe to destruct this class on any thread (which is a really hard condition to check the first time much less maintain) -- this is why I said the contained objects shouldn't be RefCounted.

Rather than just point at String, I left it more general because I didn't have time to check the rest. 

I hope that helps. Let me know if you need more. I can be more verbose now as you've noticed :).

[David Grogan]

Thanks for the explanation.  Turns out this is more than I want to bite off right now, so I'll punt on it.  Filed crbug.com/105672.