Try to make DOMStorageCache more stable

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Trunk
mozilla47
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Could fix bug 1013329 and bug 1108777 with some luck.
(Assignee)

Comment 1

4 years ago
I think the mLoaded flag is OK.  Set under the lock, and protected by function boundaries where read unprotected.

I suspect any crashes caused by DOMStorage code may be coming from sqlite.  Under some circumstances we are doing a concurrent main thread select from the database.  We can switch that off (no need to have it anyway - only small rare performance hit might occur.)


Anyway, 

bug 1013329 is WFM.

bug 1108777 might be the same case.

So, we may be OK here and this may just turn to a "check how it is -> it's OK" bug that is also WFM.
Summary: Make DOMStorageCache::mLoaded flag atomic → Try to make DOMStorageCache more stable
(In reply to Honza Bambas (:mayhemer) from comment #1)
> I think the mLoaded flag is OK.  Set under the lock, and protected by
> function boundaries where read unprotected.

FWIW, after bug 1108777 got discussed, I ran localstorage tests under TSan and it's turning up quite a few races, including one that appears to be on mLoaded.
(I'm going to file bugs after I have a chance to analyze the race reports a bit.)
(Assignee)

Comment 4

4 years ago
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #3)
> (I'm going to file bugs after I have a chance to analyze the race reports a
> bit.)

Thanks!  (I'm falling in love with TSan ;))
(Assignee)

Comment 5

3 years ago
Jan, do you think you could check on the depending bugs here and find out if those would go away with your QM work?  If not, could you take care of them (or find an assignee)?
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(jvarga)
(Assignee)

Comment 6

3 years ago
No response from Jav Varga for more then 3 weeks.  I think I'll try a very simple solution here.

Based on bug 1136922 comment 4 I think the solution here is to simply make mLoaded an atomic.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Flags: needinfo?(jvarga)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1136922
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1136926
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1136932
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1136952
(Assignee)

Comment 11

3 years ago
Created attachment 8713614 [details] [diff] [review]
v1

Using release-acquire since this is just a signaling flag.  But fully sequential might be safer here?  Not sure about the performance benefits here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a616396abe64
Attachment #8713614 - Flags: review?(nfroyd)
(Assignee)

Comment 12

3 years ago
Comment on attachment 8713614 [details] [diff] [review]
v1

Pressed enter too soon...
Attachment #8713614 - Attachment is patch: false
Attachment #8713614 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8713614 - Attachment is obsolete: true
Attachment #8713614 - Attachment is patch: true
(Assignee)

Comment 13

3 years ago
Created attachment 8713642 [details] [diff] [review]
v1

based on bug 1136922 comment 1 - 4.

- using rel-acq since this is a status flag only
- not sure this will make TSan happy tho.. however, now I'm confident access to mData is protected well enough to blacklist it from TSan's checks (not part of this patch as I need a confirmation on this first)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b22e0c1ac5c9
Attachment #8713642 - Flags: review?(nfroyd)
Comment on attachment 8713642 [details] [diff] [review]
v1

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

r=me, I suppose, assuming we're only touching mLoaded on two threads and not more.

::: dom/storage/DOMStorageCache.cpp
@@ +565,5 @@
>  DOMStorageCache::CloneFrom(const DOMStorageCache* aThat)
>  {
> +  // This will never be called on anything else than SessionStorage.
> +  // This means mData will never be touched on any other thread than
> +  // the main thread and it never went through the loading process.

Why not just assert NS_IsMainThread here, since we're touching mData below?

(I'm not really qualified to review this part of the patch, so apologies in advance of dumb questions...)

::: dom/storage/DOMStorageCache.h
@@ +222,5 @@
>    // the database (i.e. it is persistent) this flags is set to true after
>    // all keys and coresponding values are loaded from the database.
> +  // This flag never goes from true back to false.  Since this flag is
> +  // critical for mData hashtable synchronization, it's made atomic.
> +  Atomic<bool, ReleaseAcquire> mLoaded;

I think ReleaseAcquire is sufficient here (this is used for synchronization between only two threads, yes?), though the default would be better unless we have good reason to think this is very hot code...
Attachment #8713642 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 15

3 years ago
(In reply to Nathan Froyd [:froydnj] from comment #14)
> Comment on attachment 8713642 [details] [diff] [review]
> v1
> 
> Review of attachment 8713642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, I suppose, assuming we're only touching mLoaded on two threads and not
> more.

Thanks for a quick r!

Only two threads so far.  I will watch how any planned IPC changes go on, those might introduce another thread accessing mData[] and mLoaded.

> 
> ::: dom/storage/DOMStorageCache.cpp
> @@ +565,5 @@
> >  DOMStorageCache::CloneFrom(const DOMStorageCache* aThat)
> >  {
> > +  // This will never be called on anything else than SessionStorage.
> > +  // This means mData will never be touched on any other thread than
> > +  // the main thread and it never went through the loading process.
> 
> Why not just assert NS_IsMainThread here, since we're touching mData below?

We can do that, yes.

> 
> (I'm not really qualified to review this part of the patch, so apologies in
> advance of dumb questions...)
> 
> ::: dom/storage/DOMStorageCache.h
> @@ +222,5 @@
> >    // the database (i.e. it is persistent) this flags is set to true after
> >    // all keys and coresponding values are loaded from the database.
> > +  // This flag never goes from true back to false.  Since this flag is
> > +  // critical for mData hashtable synchronization, it's made atomic.
> > +  Atomic<bool, ReleaseAcquire> mLoaded;
> 
> I think ReleaseAcquire is sufficient here (this is used for synchronization
> between only two threads, yes?), though the default would be better unless
> we have good reason to think this is very hot code...

It's definitely not a hot code.  I can change it, but we sync only between two threads and I would like to save the benefits of not having a mutex on the main thread here.  I don't know the difference of rel/acq vs seq well enough to see any performance implications, so hard to decide.

Comment 16

3 years ago
(In reply to Honza Bambas (:mayhemer) from comment #5)
> Jan, do you think you could check on the depending bugs here and find out if
> those would go away with your QM work?  If not, could you take care of them
> (or find an assignee)?

Well, we're still in early stage with QM work, sorry.
(Assignee)

Comment 17

3 years ago
(In reply to Jan Varga [:janv] from comment #16)
> (In reply to Honza Bambas (:mayhemer) from comment #5)
> > Jan, do you think you could check on the depending bugs here and find out if
> > those would go away with your QM work?  If not, could you take care of them
> > (or find an assignee)?
> 
> Well, we're still in early stage with QM work, sorry.

I've fixed this.
(Assignee)

Comment 18

3 years ago
Created attachment 8714796 [details] [diff] [review]
v1.1

- added main thread assertion in clone
- mem ordering left at rel/acq
Attachment #8713642 - Attachment is obsolete: true
Attachment #8714796 - Flags: review+

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea7d83c65f5d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.