|
|
Created:
6 years, 4 months ago by Lei Zhang Modified:
6 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, miu+watch_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Project:
chromium Visibility:
Public. |
DescriptionFix a PowerSaveBlocker leak in WebContentsImpl.
BUG=399922
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288254
Patch Set 1 : #
Total comments: 1
Patch Set 2 : #
Total comments: 1
Messages
Total messages: 21 (0 generated)
If OnMediaPlayingNotification() gets called twice, the first PowerSaveBlocker gets leaked.
ping
sorry for the delay. redirecting to Avi as I'm not familiar with this code, so I'll let another owner figure it out (overloaded with CY)
https://codereview.chromium.org/425713003/diff/20001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/425713003/diff/20001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3051: delete power_save_blockers_[render_frame_message_source_][player_cookie]; Gah. I see that this fixes the problem, but can't we use linked_ptr or the like in the map so we don't have to play this game of manual memory management/
On 2014/08/07 15:30:07, Avi wrote: > https://codereview.chromium.org/425713003/diff/20001/content/browser/web_cont... > File content/browser/web_contents/web_contents_impl.cc (right): > > https://codereview.chromium.org/425713003/diff/20001/content/browser/web_cont... > content/browser/web_contents/web_contents_impl.cc:3051: delete > power_save_blockers_[render_frame_message_source_][player_cookie]; > Gah. > > I see that this fixes the problem, but can't we use linked_ptr or the like in > the map so we don't have to play this game of manual memory management/ I'll pass that on to whoever wrote the code in the first place. :-P See patch set 2.
https://codereview.chromium.org/425713003/diff/40001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/425713003/diff/40001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.h:978: typedef base::ScopedPtrHashMap<uintptr_t, PowerSaveBlockerMapEntry> Why uintptr_t and not RenderFrameHost*?
On 2014/08/07 19:36:33, Avi wrote: > https://codereview.chromium.org/425713003/diff/40001/content/browser/web_cont... > File content/browser/web_contents/web_contents_impl.h (right): > > https://codereview.chromium.org/425713003/diff/40001/content/browser/web_cont... > content/browser/web_contents/web_contents_impl.h:978: typedef > base::ScopedPtrHashMap<uintptr_t, PowerSaveBlockerMapEntry> > Why uintptr_t and not RenderFrameHost*? STL hash_map, at least on Linux, doesn't know how to hash RenderFrameHost pointers. There exists other code that does the same thing I'm doing here.
Joy. LGTM
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/425713003/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/425713003/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/425713003/40001
Message was sent while issue was closed.
Change committed as 288254 |