Closed Bug 1452950 Opened 2 years ago Closed 2 years ago

IPCBlobInputStream is declared with a thread-safe counter but logic of the class is purely non-thread-safe

Categories

(Core :: DOM: File, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mayhemer, Assigned: baku)

Details

Attachments

(1 file, 1 obsolete file)

If all the status changes and method calls are intended to be all on a single thread, the class's refcnter should not be t-safe or at least we should MOZ_DIAGNOSTIC_ASSERT single thread usage when it has to travel across threads somehow.
Attached patch thread_safe.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8972803 - Flags: review?(bugs)
Comment on attachment 8972803 [details] [diff] [review]
thread_safe.patch

mState isn't protected by the mutex.
Attachment #8972803 - Flags: review?(bugs) → review-
Attachment #8972803 - Attachment is obsolete: true
Attachment #8972875 - Flags: review?(bugs)
Comment on attachment 8972875 [details] [diff] [review]
thread_safe.patch


> NS_IMETHODIMP
> IPCBlobInputStream::GetSize(int64_t* aRetval)
> {
>-  nsCOMPtr<nsIFileMetadata> fileMetadata = do_QueryInterface(mRemoteStream);
>+  bool closed = false;
>+  nsCOMPtr<nsIFileMetadata> fileMetadata;
>+  {
>+    MutexAutoLock lock(mMutex);
>+    fileMetadata = do_QueryInterface(mRemoteStream);
>+    closed = mState == eClosed;
>+  }
>+
>   if (!fileMetadata) {
>-    return mState == eClosed ? NS_BASE_STREAM_CLOSED : NS_ERROR_FAILURE;
>+    return closed ? NS_BASE_STREAM_CLOSED : NS_ERROR_FAILURE;
>   }
> 
>   return fileMetadata->GetSize(aRetval);
> }
> 
> NS_IMETHODIMP
> IPCBlobInputStream::GetLastModified(int64_t* aRetval)
> {
>-  nsCOMPtr<nsIFileMetadata> fileMetadata = do_QueryInterface(mRemoteStream);
>+  bool closed = false;
>+  nsCOMPtr<nsIFileMetadata> fileMetadata;
>+  {
>+    MutexAutoLock lock(mMutex);
>+    fileMetadata = do_QueryInterface(mRemoteStream);
>+    closed = mState == eClosed;
>+  }
>+
>   if (!fileMetadata) {
>-    return mState == eClosed ? NS_BASE_STREAM_CLOSED : NS_ERROR_FAILURE;
>+    return closed ? NS_BASE_STREAM_CLOSED : NS_ERROR_FAILURE;
>   }
> 
>   return fileMetadata->GetLastModified(aRetval);
> }
> 
> NS_IMETHODIMP
> IPCBlobInputStream::GetFileDescriptor(PRFileDesc** aRetval)
> {
>-  nsCOMPtr<nsIFileMetadata> fileMetadata = do_QueryInterface(mRemoteStream);
>+  bool closed = false;
>+  nsCOMPtr<nsIFileMetadata> fileMetadata;
>+  {
>+    MutexAutoLock lock(mMutex);
>+    fileMetadata = do_QueryInterface(mRemoteStream);
>+    closed = mState == eClosed;
>+  }
>+
>   if (!fileMetadata) {
>-    return mState == eClosed ? NS_BASE_STREAM_CLOSED : NS_ERROR_FAILURE;
>+    return closed ? NS_BASE_STREAM_CLOSED : NS_ERROR_FAILURE;
>   }
Why this setup? couldn't you just move if (!fileMetadata) inside the scope for autolock and return early there. No need for 'closed' variable.

This all assumes the internal streams are threadsafe. Their threadsafety issues aren't exactly about this bug.
But we really need to sort this out.
Attachment #8972875 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9272151070a
Make IPCBlobInputStream thread-safe, r=smaug
https://hg.mozilla.org/mozilla-central/rev/b9272151070a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.