Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1839933002:
Ensure we don't leak ObjectProxy objects
|
Created:
4 years, 8 months ago by raymes Modified:
4 years, 8 months ago Reviewers:
piman CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure we don't leak ObjectProxy objects
Previously, if an instance was destroyed before a plugin object
had a chance to be destroyed, it would be leaked. Now we always attempt
to deallocate the object if the instance is destroyed prior to the object.
BUG=594926
Committed: https://crrev.com/0868033af81c965c393c4ca6d9ac1fc81001f593
Cr-Commit-Position: refs/heads/master@{#384148}
Patch Set 1 #
Total comments: 3
Patch Set 2 : #Messages
Total messages: 30 (13 generated)
raymes@chromium.org changed reviewers: + piman@chromium.org
Attempt #2. I ran lsan on this one to make sure.
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839933002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839933002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
https://codereview.chromium.org/1839933002/diff/1/content/renderer/pepper/plu... File content/renderer/pepper/plugin_object.cc (right): https://codereview.chromium.org/1839933002/diff/1/content/renderer/pepper/plu... content/renderer/pepper/plugin_object.cc:52: ppp_class_->Deallocate(ppp_class_data_); So, in theory, we are not supposed to call back into the plugin after the instance is deleted. Would it make sense instead to call Deallocate from PluginObject::InstanceDeleted? Also that way I don't think you need the changes in ObjectProxy. https://codereview.chromium.org/1839933002/diff/1/ppapi/proxy/ppp_class_proxy.cc File ppapi/proxy/ppp_class_proxy.cc (right): https://codereview.chromium.org/1839933002/diff/1/ppapi/proxy/ppp_class_proxy... ppapi/proxy/ppp_class_proxy.cc:174: delete reinterpret_cast<ObjectProxy*>(object); nit: static_cast
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839933002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839933002/20001
That's a better idea and also fixes the bug :)
Description was changed from ========== Ensure we don't leak ObjectProxy objects Previously, if an instance was destroyed before a plugin object had a chance to be destroyed, it would be leaked. Now we always attempt to deallocate the object even if we can't instruct the plugin that it has been deleted. BUG=594926 ========== to ========== Ensure we don't leak ObjectProxy objects Previously, if an instance was destroyed before a plugin object had a chance to be destroyed, it would be leaked. Now we always attempt to deallocate the object if the instance is destroyed prior to the object. BUG=594926 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by raymes@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839933002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839933002/20001
Message was sent while issue was closed.
Description was changed from ========== Ensure we don't leak ObjectProxy objects Previously, if an instance was destroyed before a plugin object had a chance to be destroyed, it would be leaked. Now we always attempt to deallocate the object if the instance is destroyed prior to the object. BUG=594926 ========== to ========== Ensure we don't leak ObjectProxy objects Previously, if an instance was destroyed before a plugin object had a chance to be destroyed, it would be leaked. Now we always attempt to deallocate the object if the instance is destroyed prior to the object. BUG=594926 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Ensure we don't leak ObjectProxy objects Previously, if an instance was destroyed before a plugin object had a chance to be destroyed, it would be leaked. Now we always attempt to deallocate the object if the instance is destroyed prior to the object. BUG=594926 ========== to ========== Ensure we don't leak ObjectProxy objects Previously, if an instance was destroyed before a plugin object had a chance to be destroyed, it would be leaked. Now we always attempt to deallocate the object if the instance is destroyed prior to the object. BUG=594926 Committed: https://crrev.com/0868033af81c965c393c4ca6d9ac1fc81001f593 Cr-Commit-Position: refs/heads/master@{#384148} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0868033af81c965c393c4ca6d9ac1fc81001f593 Cr-Commit-Position: refs/heads/master@{#384148}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1834223009/ by thakis@chromium.org. The reason for reverting is: Somewhat speculative; PPAPITest.Instance_LeakedObjectDestructors started falling on several bots with `Assertion failed: (false), function ~BadDestructorObject, file ../../ppapi/tests/test_instance_deprecated.cc, line 234.`. Examples: https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%2... https://build.chromium.org/p/chromium.win/builders/Win8%20GN%20%28dbg%29/buil... https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN%20%28dbg%29... https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2....
Message was sent while issue was closed.
Hmm, yeah there's a comment there: // A scriptable object that should cause a crash if its destructor is run. We // don't run the destructor for objects which the plugin leaks. This is to // prevent them doing dangerous things at cleanup time, such as executing script // or creating new objects. So we shouldn't call back into the plugin at the point the instance is deleted. I think we'll have to just delete the ObjectProxy object at that point.
Message was sent while issue was closed.
Description was changed from ========== Ensure we don't leak ObjectProxy objects Previously, if an instance was destroyed before a plugin object had a chance to be destroyed, it would be leaked. Now we always attempt to deallocate the object if the instance is destroyed prior to the object. BUG=594926 Committed: https://crrev.com/0868033af81c965c393c4ca6d9ac1fc81001f593 Cr-Commit-Position: refs/heads/master@{#384148} ========== to ========== Ensure we don't leak ObjectProxy objects Previously, if an instance was destroyed before a plugin object had a chance to be destroyed, it would be leaked. Now we always attempt to deallocate the object if the instance is destroyed prior to the object. BUG=594926 Committed: https://crrev.com/0868033af81c965c393c4ca6d9ac1fc81001f593 Cr-Commit-Position: refs/heads/master@{#384148} ==========
The CQ bit was checked by raymes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839933002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839933002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1839933002/diff/1/ppapi/proxy/ppp_class_proxy.cc File ppapi/proxy/ppp_class_proxy.cc (left): https://codereview.chromium.org/1839933002/diff/1/ppapi/proxy/ppp_class_proxy... ppapi/proxy/ppp_class_proxy.cc:223: ppp_class, class_data); piman: how about tracking these objects, either in a static std::map that lives in this file or in HostVarTracker. They would be cleaned up on instance deletion. A more hacky but easier alternative would be to just delete the pointer in PluginObject. We know for sure that it was always allocated here. I think this code could probably be unwound to pass this object directly into PluginObject and manage the lifetime there but it would be a lot of work.
On Tue, Apr 5, 2016 at 12:48 AM, <raymes@chromium.org> wrote: > > > https://codereview.chromium.org/1839933002/diff/1/ppapi/proxy/ppp_class_proxy.cc > File ppapi/proxy/ppp_class_proxy.cc (left): > > > https://codereview.chromium.org/1839933002/diff/1/ppapi/proxy/ppp_class_proxy... > ppapi/proxy/ppp_class_proxy.cc:223: ppp_class, class_data); > piman: how about tracking these objects, either in a static std::map > that lives in this file or in HostVarTracker. They would be cleaned up > on instance deletion. > > A more hacky but easier alternative would be to just delete the pointer > in PluginObject. We know for sure that it was always allocated here. I > think this code could probably be unwound to pass this object directly > into PluginObject and manage the lifetime there but it would be a lot of > work. > Thinking about it, I don't think I agree with the premise of the test - we should not leak objects, especially in the renderer. If the problem is that the objects can try to reenter PPAPI from their destructor, I would prefer disabling the interfaces (i.e. all calls fail) so that no bad things can happen? > https://codereview.chromium.org/1839933002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
