Closed
Bug 1078451
Opened 10 years ago
Closed 10 years ago
Use js_free for buffers allocated with js_malloc in ThreadSharedFloatArrayBufferList
Categories
(Core :: Web Audio, defect)
Core
Web Audio
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: ggp, Assigned: ggp)
Details
Attachments
(1 file, 2 obsolete files)
5.49 KB,
patch
|
ggp
:
review+
|
Details | Diff | Splinter Review |
ThreadSharedFloatArrayBufferList should use js_free to deallocate data coming from StealJSArrayDataIntoThreadSharedFloatArrayBufferList, since that function returns a buffer allocated through js_malloc.
Assignee | ||
Comment 1•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → ggoncalves
Comment 2•10 years ago
|
||
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-
Assignee | ||
Comment 3•10 years ago
|
||
Now with actual NULL checks.
Attachment #8500576 -
Attachment is obsolete: true
Attachment #8500596 -
Flags: review?(rjesup)
Comment 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea4be6186b5
Keywords: checkin-needed
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0ea4be6186b5
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•