pp::Buffer_Dev: Clear PP_Resource upon Map() error in Init()

pp::Buffer_Dev: Clear PP_Resource upon Map() error in Init()

Now if Map() failed, we clear |data_| and |size_|, but |pp_resource_| is
not cleared. Therefore, if the caller calls buffer.is_null() it will
return false. Since the Init() failed, buffer.is_null() should return
true.

In this CL, if Map() failed, we call Clear() to explicitly clear
|pp_resource_|.

BUG=679801
Review-Url: https://codereview.chromium.org/2629543002
Cr-Commit-Position: refs/heads/master@{#443159}
Committed: https://chromium.googlesource.com/chromium/src/+/743436752458c33f45fb41236ebedd29a69c90f3

[xhwang, 2017-01-12 00:47:25]

xhwang@chromium.org changed reviewers:
+ bbudge@chromium.org, raymes@chromium.org

[xhwang, 2017-01-12 00:47:25]

PS2 is the real fix which is a one-line fix. PS1 shows how I hack the test to
simulate a Map() failure. Unfortunately I cannot find an easy way to cause Map()
failure in real, hence no new test is added.

The BUG could be related but we are not sure whether this is the real cause.
Regardless the fix makes sense to me by its own.

raymes: PTAL
bbudge: FYI since you are OOO

[raymes, 2017-01-12 01:39:49]

Hmm, I'm not sure whether clearing the resource is the right thing to do here.
The Buffer_Dev object corresponds to a resource and there is a particular
meaning to it being null. 

Couldn't callers check the result of data() to get this information instead of
checking if it's null?

[xhwang, 2017-01-12 04:32:55]

On 2017/01/12 01:39:49, raymes wrote:
> Hmm, I'm not sure whether clearing the resource is the right thing to do here.
> The Buffer_Dev object corresponds to a resource and there is a particular
> meaning to it being null. 

Yes, but Clear() will set the resource to null and release the ownership of that
resource.

https://cs.chromium.org/chromium/src/ppapi/cpp/resource.h?rcl=1484151649&l=86
 
> Couldn't callers check the result of data() to get this information instead of
> checking if it's null?

I could, but the documentation says:

// Creates & Maps a new Buffer in the browser with the given size. The
// resulting object will be is_null() if either Create() or Map() fails.

https://cs.chromium.org/chromium/src/ppapi/cpp/dev/buffer_dev.h?rcl=0&l=23

[raymes, 2017-01-12 04:34:53]

Ah, in that case lgtm. Thanks!

[xhwang, 2017-01-12 04:35:28]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2017-01-12 04:35:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 05:21:15]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484195728302040,
"parent_rev": "4aeafe93320a3851cdebc37a86ec19770c53ade9", "commit_rev":
"743436752458c33f45fb41236ebedd29a69c90f3"}

[commit-bot: I haz the power, 2017-01-12 05:22:38]

Description was changed from

==========
pp::Buffer_Dev: Clear PP_Resource upon Map() error in Init()

Now if Map() failed, we clear |data_| and |size_|, but |pp_resource_| is
not cleared. Therefore, if the caller calls buffer.is_null() it will
return false. Since the Init() failed, buffer.is_null() should return
true.

In this CL, if Map() failed, we call Clear() to explicitly clear
|pp_resource_|.

BUG=679801
==========

to

==========
pp::Buffer_Dev: Clear PP_Resource upon Map() error in Init()

Now if Map() failed, we clear |data_| and |size_|, but |pp_resource_| is
not cleared. Therefore, if the caller calls buffer.is_null() it will
return false. Since the Init() failed, buffer.is_null() should return
true.

In this CL, if Map() failed, we call Clear() to explicitly clear
|pp_resource_|.

BUG=679801

Review-Url: https://codereview.chromium.org/2629543002
Cr-Commit-Position: refs/heads/master@{#443159}
Committed:
https://chromium.googlesource.com/chromium/src/+/743436752458c33f45fb41236ebe...
==========

[commit-bot: I haz the power, 2017-01-12 05:22:40]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/743436752458c33f45fb41236ebe...