Closed
Bug 1224659
Opened 9 years ago
Closed 9 years ago
Worker DataStore code is using ErrorResults from multiple threads
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: baku)
References
Details
Attachments
(1 file)
23.38 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
This is not supported. Depending on what's thrown on the ErrorResult (e.g. a JS exception object!) touching it on the worker thread can blow up in all sorts of interesting ways. ErrorResult usage should be thread-local
Looks like Gene is not around anymore, so let's try the reviewers from bug 949325 instead....
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8688591 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 2•9 years ago
|
||
Comment on attachment 8688591 [details] [diff] [review]
ds.patch
>+ // This method cannot fail.
Then why is it marked Throws in the IDL? Looking at this, it's implemented in JS, so it totally can fail for whatever random reason it wants to.
I would much prefer this to look like this:
if (NS_WARN_IF(rv.Failed()) {
rv.SuppressException();
}
just in case it _does_ decide to fail (e.g. because of OOM or out of stack space, or any of the other not-under-your-control reasons JS can throw).
This applies to both DataStoreGetStringRunnable and DataStoreGetReadOnlyRunnable.
In DataStoreGetRunnable::MainThreadRun, you need to rv.SuppressException().
In DataStorePutRunnable::MainThreadRun, you need to rv.SuppressException(), in two places.
In DataStoreAddRunnable::MainThreadRun, same thing, two places.
In DataStoreRemoveRunnable::MainThreadRun, need to rv.SuppressException().
Same in DataStoreClearRunnable::MainThreadRun.
Same in DataStoreSyncStoreRunnable::MainThreadRun.
Same in DataStoreGetLengthRunnable::MainThreadRun.
I'm not sure about IsFailed() vs Failed(). Either way, I guess.
r=me with the above fixed.
Attachment #8688591 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Comment 4•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•