[Oilpan] Add a GC_PLUGIN_IGNORE with a TODO comment

It is not good to keep a raw pointer to an on heap WebLocalFrameImpl
as a member variable of WebDevToolsFrontendImpl on Oilpan build.

This CL adds a GC_PLUGIN_IGNORE with a TODO comment 
to notify this issue.


BUG=509911
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201386

[peria, 2015-08-27 07:45:09]

peria@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[peria, 2015-08-27 07:45:09]

PTL.

[haraken, 2015-08-27 09:13:49]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, sigbjornf@opera.com

[haraken, 2015-08-27 09:13:50]

Sigbjorn: I need your brain :) Any advice on this?

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

https://codereview.chromium.org/1322513002/diff/1/Source/web/WebDevToolsFront...
Source/web/WebDevToolsFrontendImpl.h:65:
RawPtrWillBeWeakPersistent<WebLocalFrameImpl> m_webFrame;

This pointer looks nasty...

- We shouldn't use WeakPersistent for a pointer that doesn't have a weak
semantics (m_webFrame is a strong back pointer to the LocalFrameImpl). Thus we
shouldn't use a WeakPersistent here.

- It is not easy to move WebDevToolsFrontendImpl to the heap and use Member,
because the WebDevToolsFrontendImpl is passed over to the chromium side.

- In the first place, it's not clear to me why the raw m_webFrame pointer is
safe. How is it guaranteed that the raw pointer doesn't become stale?

[sof, 2015-08-27 09:31:38]

Yes, it looked like there is an implicit assumption/dependency on overall
lifetime of m_webFrame wrt when it is last accessed by WebDevToolsFrontendImpl
to keep the current raw pointer safe, when I looked at doing something more
comprehensive here some time ago. But I might be forgetting (or not know :) ) a
detail or two.

cf. https://codereview.chromium.org/1008163003/

[haraken, 2015-08-27 09:37:34]

On 2015/08/27 09:31:38, sof wrote:
> Yes, it looked like there is an implicit assumption/dependency on overall
> lifetime of m_webFrame wrt when it is last accessed by WebDevToolsFrontendImpl
> to keep the current raw pointer safe, when I looked at doing something more
> comprehensive here some time ago. But I might be forgetting (or not know :) )
a
> detail or two.
> 
> cf. https://codereview.chromium.org/1008163003/

Thanks, your solution sounds reasonable to me.

Until it is accepted, it would make sense to add GC_PLUGIN_IGNORE onto the raw
pointer.

[sof, 2015-08-27 09:59:52]

On 2015/08/27 09:37:34, haraken wrote:
> On 2015/08/27 09:31:38, sof wrote:
> > Yes, it looked like there is an implicit assumption/dependency on overall
> > lifetime of m_webFrame wrt when it is last accessed by
WebDevToolsFrontendImpl
> > to keep the current raw pointer safe, when I looked at doing something more
> > comprehensive here some time ago. But I might be forgetting (or not know :)
)
> a
> > detail or two.
> > 
> > cf. https://codereview.chromium.org/1008163003/
> 
> Thanks, your solution sounds reasonable to me.
> 
> Until it is accepted, it would make sense to add GC_PLUGIN_IGNORE onto the raw
> pointer.

A GC_PLUGIN_IGNORE() + TODO seems in order over WeakPersistent<>.

[peria, 2015-08-28 05:50:08]

Reverted, and put a TODO comment.

[sof, 2015-08-28 06:00:38]

lgtm

[haraken, 2015-08-28 06:03:26]

LGTM

[peria, 2015-08-28 06:09:33]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2015-08-28 06:09:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1322513002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1322513002/20001

[commit-bot: I haz the power, 2015-08-28 08:25:15]

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

[commit-bot: I haz the power, 2015-08-28 08:25:16]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[peria, 2015-08-28 08:28:51]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2015-08-28 08:29:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1322513002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1322513002/20001

[commit-bot: I haz the power, 2015-08-28 09:26:14]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201386