Override CloseContents in HeadlessWebContentsImpl Delegate so that the json/close endpoint works wi…

Override CloseContents in HeadlessWebContentsImpl Delegate so that the json/close endpoint works with headless

BUG=699404
Review-Url: https://codereview.chromium.org/2733293004
Cr-Commit-Position: refs/heads/master@{#458255}
Committed: https://chromium.googlesource.com/chromium/src/+/15ae1e89e749d6443b6caffdbc78ad24f945aa67

[Oleg Sushkov, 2017-03-08 05:43:36]

sushkov@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2017-03-08 10:47:24]

lgtm with a nit.

https://codereview.chromium.org/2733293004/diff/1/headless/lib/browser/headle...
File headless/lib/browser/headless_web_contents_impl.cc (right):

https://codereview.chromium.org/2733293004/diff/1/headless/lib/browser/headle...
headless/lib/browser/headless_web_contents_impl.cc:106:
std::vector<HeadlessWebContents*> allContents =
s/allContents/all_contents/

[Oleg Sushkov, 2017-03-13 00:28:51]

On 2017/03/08 at 10:47:24, skyostil wrote:
> lgtm with a nit.
> 
>
https://codereview.chromium.org/2733293004/diff/1/headless/lib/browser/headle...
> File headless/lib/browser/headless_web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/2733293004/diff/1/headless/lib/browser/headle...
> headless/lib/browser/headless_web_contents_impl.cc:106:
std::vector<HeadlessWebContents*> allContents =
> s/allContents/all_contents/

done

[Oleg Sushkov, 2017-03-13 00:28:57]

The CQ bit was checked by sushkov@chromium.org

[Oleg Sushkov, 2017-03-13 00:28:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2733293004/#ps20001
(title: "Override CloseContents in HeadlessWebContentsImpl Delegate so that the
json/close endpoint works wi…")

[commit-bot: I haz the power, 2017-03-13 00:29:08]

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-13 01:08:08]

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

[commit-bot: I haz the power, 2017-03-13 01:08:08]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Oleg Sushkov, 2017-03-13 06:19:01]

The CQ bit was checked by sushkov@chromium.org

[Oleg Sushkov, 2017-03-13 06:19:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2733293004/#ps40001
(title: "Fixed a nullptr issue")

[commit-bot: I haz the power, 2017-03-13 06:19:07]

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-13 06:58:02]

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

[commit-bot: I haz the power, 2017-03-13 06:58:03]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2017-03-13 12:40:28]

https://codereview.chromium.org/2733293004/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_web_contents_impl.cc (right):

https://codereview.chromium.org/2733293004/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_web_contents_impl.cc:110: if (wc == nullptr) {
nit: You can just do "if (!wc)"

[Oleg Sushkov, 2017-03-15 04:49:04]

On 2017/03/13 at 12:40:28, skyostil wrote:
>
https://codereview.chromium.org/2733293004/diff/40001/headless/lib/browser/he...
> File headless/lib/browser/headless_web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/2733293004/diff/40001/headless/lib/browser/he...
> headless/lib/browser/headless_web_contents_impl.cc:110: if (wc == nullptr) {
> nit: You can just do "if (!wc)"

Hi Sami, PTAL. My original CL was causing some problems with a few of the tests.
Upon some digging I think I've determined it was due to a circular loop caused
by called wed_contents_.Close in the HeadlessWebContentsImpl destructor. What
was happening was this:

HeadlessBrowserContextImpl destructor calls web_contents_map_.clear();
This causes a destructor call for every element of the map
This calls into HeadlessWebContentsImpl destructor
this destructor calls Close() on the web_contents
this ends up in the HeadlessWebContentsImpl::Delegate CloseContents function
(that I added)
this calls GetAllWebContents on the browser_context
But this call needs the web_Contents_map, which is currently in its destructor!

I'm not sure if this is a problem with how I've implemented CloseContents() in
the delegate, a problem with one of the destructors in the chain, or a
conceptual issue with the ownership semantics between the classes. What do you
think?

[Sami, 2017-03-15 18:19:42]

On 2017/03/15 04:49:04, Oleg Sushkov wrote:
> On 2017/03/13 at 12:40:28, skyostil wrote:
> >
>
https://codereview.chromium.org/2733293004/diff/40001/headless/lib/browser/he...
> > File headless/lib/browser/headless_web_contents_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2733293004/diff/40001/headless/lib/browser/he...
> > headless/lib/browser/headless_web_contents_impl.cc:110: if (wc == nullptr) {
> > nit: You can just do "if (!wc)"
> 
> Hi Sami, PTAL. My original CL was causing some problems with a few of the
tests.
> Upon some digging I think I've determined it was due to a circular loop caused
> by called wed_contents_.Close in the HeadlessWebContentsImpl destructor. What
> was happening was this:
> 
> HeadlessBrowserContextImpl destructor calls web_contents_map_.clear();
> This causes a destructor call for every element of the map
> This calls into HeadlessWebContentsImpl destructor
> this destructor calls Close() on the web_contents
> this ends up in the HeadlessWebContentsImpl::Delegate CloseContents function
> (that I added)
> this calls GetAllWebContents on the browser_context
> But this call needs the web_Contents_map, which is currently in its
destructor!
> 
> I'm not sure if this is a problem with how I've implemented CloseContents() in
> the delegate, a problem with one of the destructors in the chain, or a
> conceptual issue with the ownership semantics between the classes. What do you
> think?

Seems to me like the problem is that ~HeadlessBrowserContextImpl isn't shutting
down atomically. One fix I can think of is std::swapping web_contents_map_ with
an empty map and then clearing it. That way no-one can see a partially
destructed state. WDYT?

[Oleg Sushkov, 2017-03-15 23:51:05]

I question the need to call "Close" in the ~HeadlessWebContentsImpl(). When
you look at the ownership semantics of
HeadlessWebContents/HeadlessWebContentsImpl, it is "owned" by the
HeadlessBrowserContextImpl. This ownership is through holding unique_ptrs
in the web_contents_map_. Nothing else in the code (from what I can see)
holds owned instances of HeadlessWebContentsImpl.

Now with this in mind, when we destruct a HeadlessWebContentsImpl and call
Close() on the web_contents, this eventually ends up
in HeadlessWebContentsImpl.Close() which simply
calls browser_context()->DestroyWebContents(this), and all THAT does is
remove the instance from the web_contents_ map. But the only way we could
end up down this code path is from the HeadlessBrowserContextImpl
destructor, which is already destructing the web_contents_map. So whats the
point of removing an object from a map thats about to be destructed anyway?

-Oleg

On Thu, Mar 16, 2017 at 5:19 AM, skyostil@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2017/03/15 04:49:04, Oleg Sushkov wrote:
> > On 2017/03/13 at 12:40:28, skyostil wrote:
> > >
> >
> https://codereview.chromium.org/2733293004/diff/40001/
> headless/lib/browser/headless_web_contents_impl.cc
> > > File headless/lib/browser/headless_web_contents_impl.cc (right):
> > >
> > >
> >
> https://codereview.chromium.org/2733293004/diff/40001/
> headless/lib/browser/headless_web_contents_impl.cc#newcode110
> > > headless/lib/browser/headless_web_contents_impl.cc:110: if (wc ==
> nullptr) {
> > > nit: You can just do "if (!wc)"
> >
> > Hi Sami, PTAL. My original CL was causing some problems with a few of the
> tests.
> > Upon some digging I think I've determined it was due to a circular loop
> caused
> > by called wed_contents_.Close in the HeadlessWebContentsImpl destructor.
> What
> > was happening was this:
> >
> > HeadlessBrowserContextImpl destructor calls web_contents_map_.clear();
> > This causes a destructor call for every element of the map
> > This calls into HeadlessWebContentsImpl destructor
> > this destructor calls Close() on the web_contents
> > this ends up in the HeadlessWebContentsImpl::Delegate CloseContents
> function
> > (that I added)
> > this calls GetAllWebContents on the browser_context
> > But this call needs the web_Contents_map, which is currently in its
> destructor!
> >
> > I'm not sure if this is a problem with how I've implemented
> CloseContents() in
> > the delegate, a problem with one of the destructors in the chain, or a
> > conceptual issue with the ownership semantics between the classes. What
> do you
> > think?
>
> Seems to me like the problem is that ~HeadlessBrowserContextImpl isn't
> shutting
> down atomically. One fix I can think of is std::swapping web_contents_map_
> with
> an empty map and then clearing it. That way no-one can see a partially
> destructed state. WDYT?
>
> https://codereview.chromium.org/2733293004/
>

-- 
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.

[Sami, 2017-03-17 16:35:23]

On 2017/03/15 23:51:05, Oleg Sushkov wrote:
> I question the need to call "Close" in the ~HeadlessWebContentsImpl(). When
> you look at the ownership semantics of
> HeadlessWebContents/HeadlessWebContentsImpl, it is "owned" by the
> HeadlessBrowserContextImpl. This ownership is through holding unique_ptrs
> in the web_contents_map_. Nothing else in the code (from what I can see)
> holds owned instances of HeadlessWebContentsImpl.
> 
> Now with this in mind, when we destruct a HeadlessWebContentsImpl and call
> Close() on the web_contents, this eventually ends up
> in HeadlessWebContentsImpl.Close() which simply
> calls browser_context()->DestroyWebContents(this), and all THAT does is
> remove the instance from the web_contents_ map. But the only way we could
> end up down this code path is from the HeadlessBrowserContextImpl
> destructor, which is already destructing the web_contents_map. So whats the
> point of removing an object from a map thats about to be destructed anyway?

Right, looks like WebContents::Close just ends up telling the delegate to close,
which we are already doing anyway:

https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...

If it was doing something other than that then we'd probably still need it
(since HeadlessWebContentsImpl owns the WebContents), but given it doesn't
removing that call seems fine to me.

[Oleg Sushkov, 2017-03-20 23:41:53]

The CQ bit was checked by sushkov@chromium.org

[Oleg Sushkov, 2017-03-20 23:41:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2733293004/#ps60001
(title: "removed the call to web_contents_.Close() from the headless web
contents destructor. This call caus…")

[commit-bot: I haz the power, 2017-03-20 23:42:31]

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-21 00:36:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490053313310660,
"parent_rev": "73b7f2fce6f5ef21212bb6482fd5cfa7ec9344f6", "commit_rev":
"15ae1e89e749d6443b6caffdbc78ad24f945aa67"}

[commit-bot: I haz the power, 2017-03-21 00:37:27]

Description was changed from

==========
Override CloseContents in HeadlessWebContentsImpl Delegate so that the
json/close endpoint works with headless

BUG=699404
==========

to

==========
Override CloseContents in HeadlessWebContentsImpl Delegate so that the
json/close endpoint works with headless

BUG=699404

Review-Url: https://codereview.chromium.org/2733293004
Cr-Commit-Position: refs/heads/master@{#458255}
Committed:
https://chromium.googlesource.com/chromium/src/+/15ae1e89e749d6443b6caffdbc78...
==========

[commit-bot: I haz the power, 2017-03-21 00:37:28]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/15ae1e89e749d6443b6caffdbc78...