Pepper: Var keeps invalid var_id if VarTracker release it and there is another reference

Reland; Pepper: Var keeps invalid var_id if VarTracker release it and there is another reference

When VarTracker remove PP_Var from VarMap, it release its Var object
if needed, but never reset var_id stored in Var object. Then, if Var's
reference count is not 1, Var continue to exist with invalid var_id until
the last reference is released.

BUG=87310
TEST=ui_tests, browser_tests, nacl_integration

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114384

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114594

[Takashi Toyoshima, 2011-12-09 13:34:35]

I'm sorry but I found a bug on StringVar reference counting while I implement
straightforward C++ bindings.

Could you review this change aheads of C++ bindings?

[dmichael (off chromium), 2011-12-09 21:26:05]

http://codereview.chromium.org/8872065/diff/1/ppapi/tests/test_websocket.cc
File ppapi/tests/test_websocket.cc (right):

http://codereview.chromium.org/8872065/diff/1/ppapi/tests/test_websocket.cc#n...
ppapi/tests/test_websocket.cc:159: ReleaseVar(url);
I agree the ReleaseVar should be here. Are you sure it's not leaking now? I
think you have an extra ref-count the way it's implemented in this CL.

http://codereview.chromium.org/8872065/diff/1/webkit/plugins/ppapi/ppb_websoc...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/8872065/diff/1/webkit/plugins/ppapi/ppb_websoc...
webkit/plugins/ppapi/ppb_websocket_impl.cc:348: return
ReturnVar(close_reason_->GetPPVar());
Are you sure about this? GetPPVar already increments the ref-count (note that it
calls GetOrCreateVarId, which increments the ref-count).

[Takashi Toyoshima, 2011-12-10 01:25:52]

Only comments now.
I investigate it next week.

Thanks

http://codereview.chromium.org/8872065/diff/1/ppapi/tests/test_websocket.cc
File ppapi/tests/test_websocket.cc (right):

http://codereview.chromium.org/8872065/diff/1/ppapi/tests/test_websocket.cc#n...
ppapi/tests/test_websocket.cc:159: ReleaseVar(url);
OK, anyway it could be useful to add ref-count check assertion here.
I try it.

http://codereview.chromium.org/8872065/diff/1/webkit/plugins/ppapi/ppb_websoc...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/8872065/diff/1/webkit/plugins/ppapi/ppb_websoc...
webkit/plugins/ppapi/ppb_websocket_impl.cc:348: return
ReturnVar(close_reason_->GetPPVar());
Hum..., As you said GetPPVar seems to increment it.
OK, I need another investigation.
Actually, if I call GetCloseReason() or other returning Var interfaces twice,
the second call returns wrong value. For example, if I call it twice in
TestUninitializedPropertiesAccess, ASSERT_TRUE fails...

[Takashi Toyoshima, 2011-12-12 07:48:53]

Without ReturnVar()'s reference increment, VarTracker's NOTREACHED is fired by
invalid
reference decrement. StringVar and VarTracker seem to have a bug on reference
counting.

StringVar has no PP_Var object at its creation, and the first GetPPVar creates
actual PP_Var instance. At that time, its var_id is stored to Var.
If the created PP_Var were released, string contents keep existing in StringVar,
and only the
var_id is removed from VarTracker. I guess it intend creating new PP_Var with
existing string
contents at the next GetPPVar request. But Var object itself continues to have
vad_id as
a valid id. The next GetPPVar must create new PP_Var, but it has old invalid
var_id.
So, it will return disposed object with reference increment and the increment
fails.

I think Var's var_id is not controllable from VarTracker.
So, StringVar must start it's reference count from not 0 but 1 and decrement the
last reference
in destructor.

I have another concern for string handling.
I guess its reference counting doesn't work totally, right?
For example, the PP_Var created by StringVar::StringToPPVar()
will make a leaked StringVar to hold its content, but never released on
releasing the PP_Var.
VarTracker must have it's string content owner ship, or it must have StringVar
in their object map to release them at the last reference decrement.

I'll continue to investigate it.

[Takashi Toyoshima, 2011-12-12 10:17:49]

This second CL fixed GetPPVar()'s bug on the second PP_Var instance.
But, still StringToPPVar()'s leak exist, I think.

[Takashi Toyoshima, 2011-12-12 10:29:48]

Ah, I see.
VarTracker::AddVar handle Var's ownership. So existing code
works as I said it must do.
So, with this change, everything works fine?

[dmichael (off chromium), 2011-12-12 18:28:49]

http://codereview.chromium.org/8872065/diff/6001/ppapi/shared_impl/var.cc
File ppapi/shared_impl/var.cc (right):

http://codereview.chromium.org/8872065/diff/6001/ppapi/shared_impl/var.cc#new...
ppapi/shared_impl/var.cc:107: GetOrCreateVarID();
I think we shouldn't have this ref-count here. If the ref-count can't go to zero
while the object is alive, we get a chicken-and-egg problem and most strings
will leak, since:
1) The object can't get deleted until its reference count goes to 0.
2) The reference count can't go to zero until the object is deleted.

I think I understand the problem now. It's not exactly a bug, but it's
definitely tricky to use the current interface correctly. You're doing 'new
StringVar' in to a scoped_refptr member, which does bump the reference count in
base::RefCounted to 1 correctly. But as you note, the var id is not created, and
it's not registered with the var tracker. I'm trying to think of an elegant way
to handle this. You can certainly make it work; you just need to manage call 
GetPPVar immediately after doing "new StringVar" in PPB_WebSocket_Impl. E.g.:
empty_string_ = new StringVar("", 0);
var_.GetPPVar();  // Keep a reference while the WebSocket is alive

Then, you need to explicitly release any vars you created in the destructor:
PPB_WebSocket_Impl::~PPB_WebSocket_Impl() {
  VarTracker* var_tracker = PpapiGlobals::Get()->GetVarTracker();
  if (empty_string_.get())
    var_tracker->ReleaseVar(empty_string_.GetExistingVarID());
  if (url_.get())
    var_tracker->ReleaseVar(url_.GetExistingVarID());
  if (close_reason_.get())
    var_tracker->ReleaseVar(close_reason_.GetExistingVarID());
}

There probably needs to be a nicer & less confusing way to do this. If you were
using PP_Vars as members as you had originally (before I convinced you to use
StringVar :-p) this wouldn't have happened. I'll put some thought in to how Var
& VarTracker can get refactored...  it's debatable whether there's a bug in the
Var/VarTracker code or not, but it's at best way too easy to use incorrectly.
Now that I poked around a little, I think there's a bug in PPB_FileRef_Shared
that's similar to what you were seeing.

[brettw, 2011-12-12 18:53:40]

On Mon, Dec 12, 2011 at 10:28 AM,  <dmichael@chromium.org> wrote:
>
> http://codereview.chromium.org/8872065/diff/6001/ppapi/shared_impl/var.cc
> File ppapi/shared_impl/var.cc (right):
>
>
http://codereview.chromium.org/8872065/diff/6001/ppapi/shared_impl/var.cc#new...
> ppapi/shared_impl/var.cc:107: GetOrCreateVarID();
> I think we shouldn't have this ref-count here. If the ref-count can't go
> to zero while the object is alive, we get a chicken-and-egg problem and
> most strings will leak, since:
> 1) The object can't get deleted until its reference count goes to 0.
> 2) The reference count can't go to zero until the object is deleted.
>
> I think I understand the problem now. It's not exactly a bug, but it's
> definitely tricky to use the current interface correctly. You're doing
> 'new StringVar' in to a scoped_refptr member, which does bump the
> reference count in base::RefCounted to 1 correctly. But as you note, the
> var id is not created, and it's not registered with the var tracker. I'm
> trying to think of an elegant way to handle this. You can certainly make
> it work; you just need to manage call  GetPPVar immediately after doing
> "new StringVar" in PPB_WebSocket_Impl. E.g.:
> empty_string_ = new StringVar("", 0);
> var_.GetPPVar();  // Keep a reference while the WebSocket is alive
>
> Then, you need to explicitly release any vars you created in the
> destructor:
> PPB_WebSocket_Impl::~PPB_WebSocket_Impl() {
>  VarTracker* var_tracker = PpapiGlobals::Get()->GetVarTracker();
>  if (empty_string_.get())
>    var_tracker->ReleaseVar(empty_string_.GetExistingVarID());
>  if (url_.get())
>    var_tracker->ReleaseVar(url_.GetExistingVarID());
>  if (close_reason_.get())
>    var_tracker->ReleaseVar(close_reason_.GetExistingVarID());
> }
>
> There probably needs to be a nicer & less confusing way to do this. If
> you were using PP_Vars as members as you had originally (before I
> convinced you to use StringVar :-p) this wouldn't have happened. I'll
> put some thought in to how Var & VarTracker can get refactored...  it's
> debatable whether there's a bug in the Var/VarTracker code or not, but
> it's at best way too easy to use incorrectly. Now that I poked around a
> little, I think there's a bug in PPB_FileRef_Shared that's similar to
> what you were seeing.

Resources work the same way, and we have a ScopedPPResource which
manages making a PP_Resource for a given Resource object for some
certain scope. It may be helpful to write something similar for Vars.

The reason they work this way is that we want to be able to manage the
plugin's refcounting, which might be buggy, separately from our
internal references to objects. For example, there are cases where the
plugin can make us a resource or a var, and we store it somewhere, and
then the plugin can do a Release on it. The result will be a
resource/var with no PPAPI refcount, but one "regular" refcount.

Brett

[dmichael (off chromium), 2011-12-12 23:15:12]

On 2011/12/12 18:53:40, brettw wrote:
> On Mon, Dec 12, 2011 at 10:28 AM,  <mailto:dmichael@chromium.org> wrote:
> >
> > http://codereview.chromium.org/8872065/diff/6001/ppapi/shared_impl/var.cc
> > File ppapi/shared_impl/var.cc (right):
> >
> >
>
http://codereview.chromium.org/8872065/diff/6001/ppapi/shared_impl/var.cc#new...
> > ppapi/shared_impl/var.cc:107: GetOrCreateVarID();
> > I think we shouldn't have this ref-count here. If the ref-count can't go
> > to zero while the object is alive, we get a chicken-and-egg problem and
> > most strings will leak, since:
> > 1) The object can't get deleted until its reference count goes to 0.
> > 2) The reference count can't go to zero until the object is deleted.
> >
> > I think I understand the problem now. It's not exactly a bug, but it's
> > definitely tricky to use the current interface correctly. You're doing
> > 'new StringVar' in to a scoped_refptr member, which does bump the
> > reference count in base::RefCounted to 1 correctly. But as you note, the
> > var id is not created, and it's not registered with the var tracker. I'm
> > trying to think of an elegant way to handle this. You can certainly make
> > it work; you just need to manage call &nbsp;GetPPVar immediately after doing
> > "new StringVar" in PPB_WebSocket_Impl. E.g.:
> > empty_string_ = new StringVar("", 0);
> > var_.GetPPVar(); &nbsp;// Keep a reference while the WebSocket is alive
> >
> > Then, you need to explicitly release any vars you created in the
> > destructor:
> > PPB_WebSocket_Impl::~PPB_WebSocket_Impl() {
> > &nbsp;VarTracker* var_tracker = PpapiGlobals::Get()->GetVarTracker();
> > &nbsp;if (empty_string_.get())
> > &nbsp; &nbsp;var_tracker->ReleaseVar(empty_string_.GetExistingVarID());
> > &nbsp;if (url_.get())
> > &nbsp; &nbsp;var_tracker->ReleaseVar(url_.GetExistingVarID());
> > &nbsp;if (close_reason_.get())
> > &nbsp; &nbsp;var_tracker->ReleaseVar(close_reason_.GetExistingVarID());
> > }
> >
> > There probably needs to be a nicer & less confusing way to do this. If
> > you were using PP_Vars as members as you had originally (before I
> > convinced you to use StringVar :-p) this wouldn't have happened. I'll
> > put some thought in to how Var & VarTracker can get refactored... &nbsp;it's
> > debatable whether there's a bug in the Var/VarTracker code or not, but
> > it's at best way too easy to use incorrectly. Now that I poked around a
> > little, I think there's a bug in PPB_FileRef_Shared that's similar to
> > what you were seeing.
> 
> Resources work the same way, and we have a ScopedPPResource which
> manages making a PP_Resource for a given Resource object for some
> certain scope. It may be helpful to write something similar for Vars.
> 
> The reason they work this way is that we want to be able to manage the
> plugin's refcounting, which might be buggy, separately from our
> internal references to objects. For example, there are cases where the
> plugin can make us a resource or a var, and we store it somewhere, and
> then the plugin can do a Release on it. The result will be a
> resource/var with no PPAPI refcount, but one "regular" refcount.
> 
> Brett
Here's a CL showing how to fix PPB_FileRef that might help as an example for
PPB_WebSockets_Impl. This is without a ScopedPPVar, but I think that's a good
next step.
http://codereview.chromium.org/8917022/

[Takashi Toyoshima, 2011-12-13 08:47:08]

Now, I finally understand this resource management.
OK, my change was wrong as you said.

I think David will take refactoring on Var/VarTracker.
So, how about just reseting var_id on erasing from VarMap for now?
It's not effective on performance, but could be reasonable as a stopgap way.

I also understand GetLiveObjectsForInstance() check is useless for Var
managements.
I hope Var version of this test interface:-)

[Takashi Toyoshima, 2011-12-13 09:13:20]

I post patch set 3 which just reset var_id on erase.
I split live object check into another change.

Thank you.

[brettw, 2011-12-13 15:10:31]

lgtm

[brettw, 2011-12-13 15:11:48]

Actually, can you add a unit test for this? There's no var_tracker_unittest yet,
but you can copy the resource_tracker_unittest integration.

[dmichael (off chromium), 2011-12-13 15:34:32]

I like this approach better than what I suggested, thanks.

I agree a test would be great.

http://codereview.chromium.org/8872065/diff/11001/ppapi/shared_impl/var.h
File ppapi/shared_impl/var.h (right):

http://codereview.chromium.org/8872065/diff/11001/ppapi/shared_impl/var.h#new...
ppapi/shared_impl/var.h:69: void ResetVarID(void) { AssignVarID(0); };
nit: We don't normally put 'void' in the parameter list.

[Takashi Toyoshima, 2011-12-13 15:45:10]

Thanks,

Sure, I'll add tests for VarTracker tomorrow.
Can I split the tests into another change?
C++ API CL depends on this fix, so It could help me to work smoothly.

[dmichael (off chromium), 2011-12-13 16:05:02]

lgtm

I think it's fine to put the test in another CL.

[commit-bot: I haz the power, 2011-12-14 03:46:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/8872065/13004

[commit-bot: I haz the power, 2011-12-14 05:19:22]

Try job failure for 8872065-13004 (retry) on mac_rel for step "browser_tests"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[tommi (sloooow) - chröme, 2011-12-14 11:23:53]

fyi - this cl was reverted as the tree was very badly broken this morning.
I don't see any evidence that trybots were run successfully and the cq reported
several failures.  Why was the change committed?

[tommi (sloooow) - chröme, 2011-12-14 11:27:02]

On 2011/12/14 11:23:53, tommi wrote:
> fyi - this cl was reverted as the tree was very badly broken this morning.
> I don't see any evidence that trybots were run successfully and the cq
reported
> several failures.  Why was the change committed?

Also see this change with all red trybots:
http://codereview.chromium.org/8934003

[Takashi Toyoshima, 2011-12-14 11:56:10]

I thought the failure was not related to this change,
because it's not the tests I focused and the tests passed in local.
Then, I carelessly used dcommit to land.

8934003 depends this change as I said in IRC.
That's why revision 114397 to 114399 cause some test failures.

I'm investigating it, again.
Sorry for inconvenience on tree.

[Takashi Toyoshima, 2011-12-14 12:39:27]

tommi,

I notice that this change cause a DCHECK failure.
I'm so sorry to miss this error.

After finishing trybots, could anyone double-check my latest fix,
then grant to land again?

Thanks

[tommi1, 2011-12-14 12:44:58]

yes, no problem.  We'll open the tree in a few minutes, but you can ping me
on irc if you want to land while it's being throttled.

On Wed, Dec 14, 2011 at 1:39 PM, <toyoshim@chromium.org> wrote:

> tommi,
>
> I notice that this change cause a DCHECK failure.
> I'm so sorry to miss this error.
>
> After finishing trybots, could anyone double-check my latest fix,
> then grant to land again?
>
> Thanks
>
>
http://codereview.chromium.**org/8872065/<http://codereview.chromium.org/8872...
>

[Takashi Toyoshima, 2011-12-14 18:39:31]

Hi Brett, Antoine.

Today, I failed to land this change because it causes DCHECK false error in
AssignVarID().
AssignVarID() has a DCHECK to verify called only once, so my ResetVarID() fire
the DCHECK.
I fixed it to reset var_id_ member directly to zero in order to avoid firing
DCHECK.

It might be trivial, but I'd be happy if you double-checked this revision for
safe landing.
Thanks.

[brettw, 2011-12-14 22:18:31]

lgtm

[commit-bot: I haz the power, 2011-12-15 00:54:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/8872065/12005

[commit-bot: I haz the power, 2011-12-15 02:11:37]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2011-12-15 02:20:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/8872065/12005

[commit-bot: I haz the power, 2011-12-15 05:04:07]

Change committed as 114594