Oilpan: move WebDevToolsFrontend to the heap.

Oilpan: move WebDevToolsFrontend to the heap.

Make the embedder-owned WebDevToolsFrontend object wrap the underlying
Blink implementation behind a WebPrivatePtr<>. That enables moving
WebDevToolsFrontendImpl to the Oilpan heap, having its references to other
heap objects be traced correctly.

R=
BUG=340522

[sof, 2015-03-15 14:10:55]

sigbjornf@opera.com changed reviewers:
+ dgozman@chromium.org, oilpan-reviews@chromium.org, pfeldman@chromium.org

[sof, 2015-03-15 14:10:56]

please take a look.

https://codereview.chromium.org/1008163003/diff/1/Source/web/WebDevToolsFront...
File Source/web/WebDevToolsFrontendImpl.cpp (right):

https://codereview.chromium.org/1008163003/diff/1/Source/web/WebDevToolsFront...
Source/web/WebDevToolsFrontendImpl.cpp:55: // Q: where is this cleared?
An explicit dispose() was removed in https://codereview.chromium.org/900543003/
, which did clear this ref. That review (or its commit message) doesn't offer an
indication as to why we should assume that this object will never be
finalized/destructed...as I cannot see where else it would be cleared, but
perhaps i'm overlooking something.

[pfeldman, 2015-03-15 14:33:32]

How is this related to the DOM hierarchy bug? What isbthe rationale of this? We
are thinking to go the opposite way in our agent classes, so I'd like to better
understand why the public web class that is a webui-alike embedder needs to go
oilpan.

[sof, 2015-03-15 14:41:08]

On 2015/03/15 14:33:32, pfeldman wrote:
> How is this related to the DOM hierarchy bug? What isbthe rationale of this?
We
> are thinking to go the opposite way in our agent classes, so I'd like to
better
> understand why the public web class that is a webui-alike embedder needs to go
> oilpan.

The WebLocalFrameImpl reference on WebDevToolsFrontendImpl is a raw pointer to
an Oilpan heap object. This is in general unsafe and something we want to avoid.

Is m_devtoolsHost cleared?

[pfeldman, 2015-03-15 15:05:11]

> The WebLocalFrameImpl reference on WebDevToolsFrontendImpl is a raw pointer to
> an Oilpan heap object. This is in general unsafe and something we want to
avoid.
>

But you will need to point to the heap object at some point on the way from
content/renderer?

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/d...

Also, WebDevToolsFrontendPrivate looks to me like a pure boilerplate and
indirection.

> Is m_devtoolsHost cleared?

I don't see that happening.

[sof, 2015-03-15 15:52:39]

On 2015/03/15 15:05:11, pfeldman wrote:
> > The WebLocalFrameImpl reference on WebDevToolsFrontendImpl is a raw pointer
to
> > an Oilpan heap object. This is in general unsafe and something we want to
> avoid.
> >
> 
> But you will need to point to the heap object at some point on the way from
> content/renderer?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/d...
> 

We need to keep it alive from the renderer side, but Oilpan is local to Blink
and will not trace anything kept by the embedder. Which is why the
WebPrivatePtr<> is introduced here, so as to be workable with & without Oilpan.
It takes care of the Blink finalization details for the embedder.

If we don't account for these raw pointers properly, besides the safety issue,
it prevents moving to a garbage collector infrastructure that supports movable
objects, for instance.

> Also, WebDevToolsFrontendPrivate looks to me like a pure boilerplate and
> indirection.
> 

It is, but the convention appears to be not to expose implementation objects
directly as WebPrivatePtr<>, but opaque objects with Private suffixes. So I
followed e.g., WebSecurityOrigin(Private)'s lead here. If there's another
convention used & acceptable, I'd happily switch.

> > Is m_devtoolsHost cleared?
> 
> I don't see that happening.

Do you implicitly assume that the frontend renderer process will just be torn
down without finalizing its objects?

[pfeldman, 2015-03-15 17:57:58]

> We need to keep it alive from the renderer side, but Oilpan is local to Blink
> and will not trace anything kept by the embedder. Which is why the
> WebPrivatePtr<> is introduced here, so as to be workable with & without
Oilpan.
> It takes care of the Blink finalization details for the embedder.

Is the amount of the code and complexity added justified though? We've lived
without that code before, why do we need it now?

> If we don't account for these raw pointers properly, besides the safety issue,

Could you elaborate on the safety risks or link to a bug that is currently
present?

> it prevents moving to a garbage collector infrastructure that supports movable
> objects, for instance.

Could you elaborate on what this means? You are throwing the
RefCountedWillBeGarbageCollectedFinalized + WebPrivatePtr complexity at the code
maintainers and I'd like to make sure it is justified. In either case, the
WebDevToolsFrontend that embeds  WebDevToolsFrontendPrivate that inherits from
WebDevToolsFrontendImpl needs to be explained in the great detail in the
comments. I had an interface facing public/web along with its implementation,
now my interface is a concrete class and there is a private layer in the middle.
If that is something that happens to the entire public/web layer inevitably, I'd
like to see a design doc explaining the generic approach.

> It is, but the convention appears to be not to expose implementation objects
> directly as WebPrivatePtr<>, but opaque objects with Private suffixes. So I
> followed e.g., WebSecurityOrigin(Private)'s lead here. If there's another
> convention used & acceptable, I'd happily switch.

Could you point to where Private is used to resolve the Oilpan specific
constraints?

> Do you implicitly assume that the frontend renderer process will just be torn
> down without finalizing its objects?

I think this is an oversight which did not surface due to the front-end going
away along with the process.

[sof, 2015-03-15 19:19:10]

On 2015/03/15 17:57:58, pfeldman wrote:
> > We need to keep it alive from the renderer side, but Oilpan is local to
Blink
> > and will not trace anything kept by the embedder. Which is why the
> > WebPrivatePtr<> is introduced here, so as to be workable with & without
> Oilpan.
> > It takes care of the Blink finalization details for the embedder.
> 
> Is the amount of the code and complexity added justified though? We've lived
> without that code before, why do we need it now?
> 
> > If we don't account for these raw pointers properly, besides the safety
issue,
> 
> Could you elaborate on the safety risks or link to a bug that is currently
> present?
> 

What would happen if the WebLocalFrameImpl* object is not referenced by any
other traceable reference, is swept&finalized by the GC, and then attempted
accessed via WebDevToolsFrontend from the embedder? UAF is what. Do you want to
argue that this is safe regardless and something the codebase should simply
accept? You don't seem to do similarly for the DevToolsHost reference, where you
keep a Persistent reference.

Until that has been clarified, I don't think it merits moving forward here. I'm
just trying to do the codebase a favor, Oilpan or not.

[pfeldman, 2015-03-15 19:40:38]

>> I'm just trying to do the codebase a favor, Oilpan or not.

Sorry, it did not sound to me like that, it clearly sounded "Oilpan: move
WebDevToolsFrontend to the heap." as a part of "Move DOM objects to oilpan".

Could we make a step back? I just filed a bug
https://code.google.com/p/chromium/issues/detail?id=467407 that explains what
the problem is. I hope it is correct, feel free to add details / correct me if
something is wrong. From there, I'd like to verify if we are a subject to UAF.
If that is the case (or if checking it is not trivial), we will consider one of
the two options. It is not obvious to me whether the switch from non-heap to
heap should happen on the web/ boundary or inside WDTFI. Considering the WDFI
role, I'd rather say the switch should happen inside of it.

[haraken, 2015-03-15 23:41:34]

On 2015/03/15 17:57:58, pfeldman wrote:
> > We need to keep it alive from the renderer side, but Oilpan is local to
Blink
> > and will not trace anything kept by the embedder. Which is why the
> > WebPrivatePtr<> is introduced here, so as to be workable with & without
> Oilpan.
> > It takes care of the Blink finalization details for the embedder.
> 
> Is the amount of the code and complexity added justified though? We've lived
> without that code before, why do we need it now?
> 
> > If we don't account for these raw pointers properly, besides the safety
issue,
> 
> Could you elaborate on the safety risks or link to a bug that is currently
> present?
> 
> > it prevents moving to a garbage collector infrastructure that supports
movable
> > objects, for instance.
> 
> Could you elaborate on what this means? You are throwing the
> RefCountedWillBeGarbageCollectedFinalized + WebPrivatePtr complexity at the
code
> maintainers and I'd like to make sure it is justified. In either case, the
> WebDevToolsFrontend that embeds  WebDevToolsFrontendPrivate that inherits from
> WebDevToolsFrontendImpl needs to be explained in the great detail in the
> comments. I had an interface facing public/web along with its implementation,
> now my interface is a concrete class and there is a private layer in the
middle.
> If that is something that happens to the entire public/web layer inevitably,
I'd
> like to see a design doc explaining the generic approach.

I agree that this CL is adding complexity to WebDevToolsFrontend.h, but at least
this CL makes WebDevToolsFrontend.h consistent with other web/ classes. The CL
looks reasonable to me.

I guess the underlying problem is the complexity of the content/core/web
dependency (rather than Oilpan). I'm planning to write a design doc of the
dependency reorganization after the repository merge this week. In that world,
WebPrivatePtr will be gone, hopefully.

[sof, 2015-03-20 09:20:32]

On 2015/03/15 23:41:34, haraken wrote:
> On 2015/03/15 17:57:58, pfeldman wrote:
> > > We need to keep it alive from the renderer side, but Oilpan is local to
> Blink
> > > and will not trace anything kept by the embedder. Which is why the
> > > WebPrivatePtr<> is introduced here, so as to be workable with & without
> > Oilpan.
> > > It takes care of the Blink finalization details for the embedder.
> > 
> > Is the amount of the code and complexity added justified though? We've lived
> > without that code before, why do we need it now?
> > 
> > > If we don't account for these raw pointers properly, besides the safety
> issue,
> > 
> > Could you elaborate on the safety risks or link to a bug that is currently
> > present?
> > 
> > > it prevents moving to a garbage collector infrastructure that supports
> movable
> > > objects, for instance.
> > 
> > Could you elaborate on what this means? You are throwing the
> > RefCountedWillBeGarbageCollectedFinalized + WebPrivatePtr complexity at the
> code
> > maintainers and I'd like to make sure it is justified. In either case, the
> > WebDevToolsFrontend that embeds  WebDevToolsFrontendPrivate that inherits
from
> > WebDevToolsFrontendImpl needs to be explained in the great detail in the
> > comments. I had an interface facing public/web along with its
implementation,
> > now my interface is a concrete class and there is a private layer in the
> middle.
> > If that is something that happens to the entire public/web layer inevitably,
> I'd
> > like to see a design doc explaining the generic approach.
> 
> I agree that this CL is adding complexity to WebDevToolsFrontend.h, but at
least
> this CL makes WebDevToolsFrontend.h consistent with other web/ classes. The CL
> looks reasonable to me.
> 
> I guess the underlying problem is the complexity of the content/core/web
> dependency (rather than Oilpan). I'm planning to write a design doc of the
> dependency reorganization after the repository merge this week. In that world,
> WebPrivatePtr will be gone, hopefully.

If there is a simpler/more-compliant way to handle the keepalive reference back
to WebDevToolsFrontendImpl, then please let me know.

[pfeldman, 2015-05-15 23:50:54]

pfeldman@chromium.org changed reviewers:
- pfeldman@chromium.org

[dgozman, 2015-06-30 14:25:02]

dgozman@chromium.org changed reviewers:
- dgozman@chromium.org