Closed Bug 1078451 Opened 5 years ago Closed 5 years ago

Use js_free for buffers allocated with js_malloc in ThreadSharedFloatArrayBufferList

Categories

(Core :: Web Audio, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: ggp, Assigned: ggp)

Details

Attachments

(1 file, 2 obsolete files)

ThreadSharedFloatArrayBufferList should use js_free to deallocate data coming from StealJSArrayDataIntoThreadSharedFloatArrayBufferList, since that function returns a buffer allocated through js_malloc.
One option is to add another parameter to ThreadSharedFloatArrayBufferList::SetData,
allowing callers to specify the function that will be used to free the data.

:jesup had also suggested keeping JS buffers in a separate member inside
ThreadSharedFloatArrayBufferList::Storage so it can be js_free'd, and using a separate
setter for that member. Please feel free to r- if you think that option is preferable
over the extra parameter.
Attachment #8500576 - Flags: review?(rjesup)
Assignee: nobody → ggoncalves
Comment on attachment 8500576 [details] [diff] [review]
Accept a free function in ThreadSharedFloatArrayBufferList::SetData

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

::: content/media/webaudio/AudioNodeEngine.h
@@ +72,3 @@
>    {
>      Storage* s = &mContents[aIndex];
> +    s->mFree(s->mDataToFree);

The first time this is called, mFree (and mDataToFree) may be null!

if (s->mFree) {
  s->mFree(...);
} else {
  MOZ_ASSERT(!s->mDataToFree);
}
or some such.

Also missing:
   s->mFree = aFreeFunc;
Attachment #8500576 - Flags: review?(rjesup) → review-
Now with actual NULL checks.
Attachment #8500576 - Attachment is obsolete: true
Attachment #8500596 - Flags: review?(rjesup)
Comment on attachment 8500596 [details] [diff] [review]
Accept a free function in ThreadSharedFloatArrayBufferList::SetData

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

r+ with nits

::: content/media/webaudio/AudioNodeEngine.h
@@ +45,1 @@
>        mSampleData = nullptr;

Storage() :
     mDataToFree(nullptr),
     mFree(nullptr),
     mSampleData(nullptr)
   {}

@@ +46,5 @@
>      }
> +    ~Storage() {
> +      if (mFree) {
> +        mFree(mDataToFree);
> +      }

else { MOZ_ASSERT(!mDataToFree); }
Attachment #8500596 - Flags: review?(rjesup) → review+
Nits fixed, carrying over r+. Thanks for the quick review!

https://tbpl.mozilla.org/?tree=Try&rev=db8adc9fbc12
Attachment #8500596 - Attachment is obsolete: true
Attachment #8501187 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ea4be6186b5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.