Oilpan: move ScriptController+WindowProxy to the heap.

Oilpan: move ScriptController+WindowProxy to the heap.

Turn LocalFrame's ScriptController into a GCed object, letting it
correctly trace its member reference back to its owner.

Having it be heap allocated also simplifies how its LocalFrame is kept
alive across calls that invoke scripts which indirectly detaches/removes
the frame.

Transitively do the exact same for ScriptController's owned WindowProxies.

R=mkwst,haraken
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183255

[sof, 2014-10-05 15:20:42]

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

[sof, 2014-10-05 15:20:43]

Please take a look.

See
https://groups.google.com/a/chromium.org/forum/#!topic/oilpan-reviews/JbTaFVk...
for some context on why this change is being made.

[Mike West, 2014-10-05 16:25:44]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-10-05 16:25:45]

LGTM. It's probably a good idea to get an oilpanner to take a second pass, but
this looks right to me (and the bots seem happy).

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WindowProxy.h (left):

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/WindowProxy.h:105: static WindowProxy*
enteredIsolatedWorldContext();
Was this undefined? Weird.

https://codereview.chromium.org/627933002/diff/60001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/627933002/diff/60001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.h:107: ScriptController& script() const;
Good catch.

[haraken, 2014-10-05 23:37:25]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2014-10-05 23:37:26]

LGTM

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptController.cpp:241:
OwnPtrWillBeMember<WindowProxy> isolatedWorldWindowProxy =
WindowProxy::create(m_frame, world, m_isolate);

OwnPtrWillBeRawPtr ?

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptController.h (right):

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptController.h:158: typedef HashMap<Widget*,
NPObject*> PluginObjectMap;

Just to confirm: Are you sure that these raw pointers don't become stale
pointers when the ScriptController (on-heap) outlives Widgets or NPObjects
(off-heap)?

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/WindowProxy.cpp:91: // WindowProxy::clearForClose() must
be invoked before destruction starts.

WindowProxy::clearForClose() => WindowProxy::clearForClose() or
WindowProxy::clearForNavigation().

or

WindowProxy::clearForClose() => WindowProxy::disposeContext()

[sof, 2014-10-06 07:34:40]

Thanks for the reviews.

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptController.cpp:241:
OwnPtrWillBeMember<WindowProxy> isolatedWorldWindowProxy =
WindowProxy::create(m_frame, world, m_isolate);
On 2014/10/05 23:37:26, haraken wrote:
> 
> OwnPtrWillBeRawPtr ?

Thanks, fixed up.

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptController.h (right):

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptController.h:158: typedef HashMap<Widget*,
NPObject*> PluginObjectMap;
On 2014/10/05 23:37:26, haraken wrote:
> 
> Just to confirm: Are you sure that these raw pointers don't become stale
> pointers when the ScriptController (on-heap) outlives Widgets or NPObjects
> (off-heap)?

Nothing changes wrt that; we do rely on plugins to clear out and unregister
their scripts (via the plugin container.) And that will still happen _and_
happen before this ScriptController (and its LocalFrame) is finalized.

I think this will remain workable once Widget (=> the plugin container) is moved
to the heap. If not, there's (even) more work to attend to there!

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/WindowProxy.cpp:91: // WindowProxy::clearForClose() must
be invoked before destruction starts.
On 2014/10/05 23:37:26, haraken wrote:
> 
> WindowProxy::clearForClose() => WindowProxy::clearForClose() or
> WindowProxy::clearForNavigation().
> 
> or
> 
> WindowProxy::clearForClose() => WindowProxy::disposeContext()

Thanks, updated.

[haraken, 2014-10-06 07:39:23]

>
https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
> File Source/bindings/core/v8/ScriptController.h (right):
> 
>
https://codereview.chromium.org/627933002/diff/60001/Source/bindings/core/v8/...
> Source/bindings/core/v8/ScriptController.h:158: typedef HashMap<Widget*,
> NPObject*> PluginObjectMap;
> On 2014/10/05 23:37:26, haraken wrote:
> > 
> > Just to confirm: Are you sure that these raw pointers don't become stale
> > pointers when the ScriptController (on-heap) outlives Widgets or NPObjects
> > (off-heap)?
> 
> Nothing changes wrt that; we do rely on plugins to clear out and unregister
> their scripts (via the plugin container.) And that will still happen _and_
> happen before this ScriptController (and its LocalFrame) is finalized.
> 
> I think this will remain workable once Widget (=> the plugin container) is
moved
> to the heap. If not, there's (even) more work to attend to there!

Makes sense!

[sof, 2014-10-06 07:52:08]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-10-06 07:52:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/627933002/80001

[commit-bot: I haz the power, 2014-10-06 08:58:42]

Committed patchset #5 (id:80001) as 183255