Fix WebStateListObserver::WebStateDetachedAt() parameter lifetime.

Fix WebStateListObserver::WebStateDetachedAt() parameter lifetime.

The WebState pointer passed to WebStateListObserver::WebStateDetachedAt()
was a pointer to deallocated memory if WebStateList managed the ownership
of the WebState because destroying the WebStateListWrapper deallocated it.

Change WebStateList::DetachWebStateAt() to release the ownership of the
WebState and mark the method (and WebStateList::ReplaceWebStateAt) with
WARN_UNUSED_RESULT so that compiler ensure client code uses the returned
value (by passing it to a std::unique_ptr<> for example).

Change how the lifetime of the WebState is managed in WebStateList to
avoid making the same class of errors in the future (still not optimal
as the best would be to use std::unique_ptr<> but it is not possible
until Tab* and WebState* ownership is reversed).

BUG=687207
Review-Url: https://codereview.chromium.org/2710183003
Cr-Commit-Position: refs/heads/master@{#453790}
Committed: https://chromium.googlesource.com/chromium/src/+/977ff883dfd769b8b79bdb73ed3544b5fa971abb

[sdefresne, 2017-02-23 14:40:08]

The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 14:40:18]

Dry run: 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-02-23 15:09:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 15:09:55]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-02-23 16:46:01]

sdefresne@chromium.org changed reviewers:
+ rohitrao@chromium.org

[sdefresne, 2017-02-23 16:46:02]

Please take a look and send to CQ if LGTY.

[sdefresne, 2017-02-24 10:55:23]

rohitrao: ping

[sdefresne, 2017-02-27 18:25:10]

sdefresne@chromium.org changed reviewers:
+ pkl@chromium.org

[sdefresne, 2017-02-27 18:25:10]

Peter: can you take a look too?

Note that the WebStateList cannot yet always owns the WebState objects (as it is
used by both the new and old architecture and in the old architecture the
objects are owned by Tab).

Here this CL fixes a bug when WebStateList owns the WebState. The bug was that
WebStateListObserver::WebStateDetachedAt() was called with a pointer to a
WebState that had been deallocated.

I fixed the bug by moving the code that delete the WebState from WebStateWrapper
to WebStateList destructor. This means that the method that remove a WebState
from the WebStateList (i.e. DetachWebState and ReplaceWebState) returns the
removed WebState and give the ownership to the caller (if WebState owned the
pointer).

The correct way to do this would have been to return a std::unique_ptr<> but
since WebStateList does not owns the pointer in the old architecture, I must
return a raw pointer and the caller need to wrap it in a std::unique_ptr<>
itself.

This is a temporary situation. I'm working on changing the ownership
relationship between Tab and WebState. At that point, I'll update the API to
return a std::unique_ptr<> and the code will be safe.

In the meantime, to limit the risk of bugs, I've used a macro that cause a
compilation error if the returned value from those two methods is not used, and
used ignore_result() / std::unique_ptr<> constructor to mark the value as used
(ignore_result in the old architecture, std::unique_ptr<> in the new one).

[pkl (ping after 24h if needed), 2017-02-27 19:32:10]

LGTM

https://codereview.chromium.org/2710183003/diff/1/ios/shared/chrome/browser/t...
File ios/shared/chrome/browser/tabs/web_state_list.mm (right):

https://codereview.chromium.org/2710183003/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list.mm:22:
WebStateWrapper(web::WebState* web_state);
I think the style guide mandates "explicit" for single arg constructors.

[sdefresne, 2017-02-28 05:51:18]

The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 05:51:42]

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

[sdefresne, 2017-02-28 05:52:52]

https://codereview.chromium.org/2710183003/diff/1/ios/shared/chrome/browser/t...
File ios/shared/chrome/browser/tabs/web_state_list.mm (right):

https://codereview.chromium.org/2710183003/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list.mm:22:
WebStateWrapper(web::WebState* web_state);
On 2017/02/27 19:32:09, pklpkl wrote:
> I think the style guide mandates "explicit" for single arg constructors.

Done. Thank you.

[commit-bot: I haz the power, 2017-02-28 06:09:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 06:09:41]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-02-28 18:09:33]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2017-02-28 18:09:34]

The patchset sent to the CQ was uploaded after l-g-t-m from pkl@chromium.org
Link to the patchset: https://codereview.chromium.org/2710183003/#ps20001
(title: "Mark WebStateList::WebStateWrapper constructor as explicit.")

[commit-bot: I haz the power, 2017-02-28 18:10:04]

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-02-28 18:10:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 18:10:09]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[rohitrao (ping after 24h), 2017-02-28 22:35:58]

lgtm

https://codereview.chromium.org/2710183003/diff/20001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab_model.mm (right):

https://codereview.chromium.org/2710183003/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab_model.mm:631: // the result and won't cause a memory
leak. Once the ownershipd is moved
Typo: ownership.

https://codereview.chromium.org/2710183003/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab_model.mm:802: // the result and won't cause a memory
leak. Once the ownershipd is moved
Typo: ownership.

[sdefresne, 2017-03-01 01:22:53]

https://codereview.chromium.org/2710183003/diff/20001/ios/chrome/browser/tabs...
File ios/chrome/browser/tabs/tab_model.mm (right):

https://codereview.chromium.org/2710183003/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab_model.mm:631: // the result and won't cause a memory
leak. Once the ownershipd is moved
On 2017/02/28 22:35:58, rohitrao wrote:
> Typo: ownership.

Done.

https://codereview.chromium.org/2710183003/diff/20001/ios/chrome/browser/tabs...
ios/chrome/browser/tabs/tab_model.mm:802: // the result and won't cause a memory
leak. Once the ownershipd is moved
On 2017/02/28 22:35:58, rohitrao wrote:
> Typo: ownership.

Done.

[sdefresne, 2017-03-01 01:23:11]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2017-03-01 01:23:14]

The patchset sent to the CQ was uploaded after l-g-t-m from pkl@chromium.org,
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2710183003/#ps40001
(title: "Fix typos.")

[commit-bot: I haz the power, 2017-03-01 01:23:36]

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-03-01 01:35:56]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488331391102350,
"parent_rev": "0f396eed478d22127dfca39aa1e6631c51d093f7", "commit_rev":
"977ff883dfd769b8b79bdb73ed3544b5fa971abb"}

[commit-bot: I haz the power, 2017-03-01 01:37:20]

Description was changed from

==========
Fix WebStateListObserver::WebStateDetachedAt() parameter lifetime.

The WebState pointer passed to WebStateListObserver::WebStateDetachedAt()
was a pointer to deallocated memory if WebStateList managed the ownership
of the WebState because destroying the WebStateListWrapper deallocated it.

Change WebStateList::DetachWebStateAt() to release the ownership of the
WebState and mark the method (and WebStateList::ReplaceWebStateAt) with
WARN_UNUSED_RESULT so that compiler ensure client code uses the returned
value (by passing it to a std::unique_ptr<> for example).

Change how the lifetime of the WebState is managed in WebStateList to
avoid making the same class of errors in the future (still not optimal
as the best would be to use std::unique_ptr<> but it is not possible
until Tab* and WebState* ownership is reversed).

BUG=687207
==========

to

==========
Fix WebStateListObserver::WebStateDetachedAt() parameter lifetime.

The WebState pointer passed to WebStateListObserver::WebStateDetachedAt()
was a pointer to deallocated memory if WebStateList managed the ownership
of the WebState because destroying the WebStateListWrapper deallocated it.

Change WebStateList::DetachWebStateAt() to release the ownership of the
WebState and mark the method (and WebStateList::ReplaceWebStateAt) with
WARN_UNUSED_RESULT so that compiler ensure client code uses the returned
value (by passing it to a std::unique_ptr<> for example).

Change how the lifetime of the WebState is managed in WebStateList to
avoid making the same class of errors in the future (still not optimal
as the best would be to use std::unique_ptr<> but it is not possible
until Tab* and WebState* ownership is reversed).

BUG=687207

Review-Url: https://codereview.chromium.org/2710183003
Cr-Commit-Position: refs/heads/master@{#453790}
Committed:
https://chromium.googlesource.com/chromium/src/+/977ff883dfd769b8b79bdb73ed35...
==========

[commit-bot: I haz the power, 2017-03-01 01:37:20]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/977ff883dfd769b8b79bdb73ed35...