Memory leak and dangling pointer in WebRtcLogUploader |
||
Issue description1. WebRtcLogUploader::UploadCompressedLog allocates a URLFetcher on the stack. It then proceeds to Post() it as Unretained(), which risks leaking memory if the task does not excute. 2. More importantly, if WebRtcLogUploader::ShutdownOnIOThread() is called, upload_done_data_.clear() will be called. This not delete the fetcher, which means that the OnURLFetchComplete() can still be called on the object, even if it ends up being deleted - which would normally be the case after ShutdownOnIOThread().
,
May 23 2018
It is not clear to me that ShutdownOnIOThread is guaranteed to run before the destructor. That's a potential issue which I'll need to corroborate or disprove before this bug can be closed.
,
May 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b0cfcf9d195f91718f99a4762d080c25e9751b75 commit b0cfcf9d195f91718f99a4762d080c25e9751b75 Author: Elad Alon <eladalon@chromium.org> Date: Wed May 23 19:46:35 2018 Avoid raw-pointers in tasks posted by WebRtcTextLogUploader By using unique_ptr in the bound callback, we can make sure that memory won't be leaked if the task is dropped. Bug: 845456 Change-Id: If905e503c798a748ad27076449bf400ae91a97d7 Reviewed-on: https://chromium-review.googlesource.com/1070149 Commit-Queue: Elad Alon <eladalon@chromium.org> Reviewed-by: Tommi <tommi@chromium.org> Cr-Commit-Position: refs/heads/master@{#561215} [modify] https://crrev.com/b0cfcf9d195f91718f99a4762d080c25e9751b75/chrome/browser/media/webrtc/webrtc_log_uploader.cc [modify] https://crrev.com/b0cfcf9d195f91718f99a4762d080c25e9751b75/chrome/browser/media/webrtc/webrtc_log_uploader.h
,
May 23 2018
The comments on the landed CL contain the explanation of why the ShutdownOnIOThread part is not a problem. Resolving this bug now.
,
Jun 7 2018
[bulk-edit: disregard if N/A] Can the owner please set milestone to this bug if applicable? |
||
►
Sign in to add a comment |
||
Comment 1 by eladalon@chromium.org
, May 23 2018Ah, I have missed this: // Delete all URLFetchers first and clear the upload done map. for (const auto& it : upload_done_data_) delete it.first;