Oilpan: have PendingScripts trace their script elements.

Oilpan: have PendingScripts trace their script elements.

To complete the move to transition types for Element in html/ and dom/,
convert PendingScript and the script runner objects to trace their
embedded script element references. The script runner objects
(ScriptRunner and HTMLScriptRunner) are now garbage collected objects,
with PendingScript being kept in-line or as a part objects of those
runner objects.

To enable detaching of HTMLScriptRunner without having to rely on its
host (HTMLDocumentParser), the HTMLScriptRunner is instead made the
client of the resources it loads. This simplifies dependencies, but
requiring some follow-on changes to the HTMLScriptRunnerHost interface.

Besides script element references, convert a few other remaining Element
{Pass}RefPtr uses to transition types.

R=ager@chromium.org,haraken@chromium.org,eseidel@chromium.org
BUG=357163
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175127

[sof, 2014-05-25 16:39:31]

Please take a look.

I think I'm comfortable with PendingScript as handled here,
but perhaps there are better ways? Suggestions welcome.

[haraken, 2014-05-26 01:08:24]

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/PendingS...
File Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/PendingS...
Source/core/dom/PendingScript.h:113: // PendingScript off heap, along with its
immediate owner.

Hmm, can we probably address the issue using weak processing?

By calling registerWeakMembers(this, weakCallback), the weak processing is
called back when the |this| object is going to die. In the weak processing,
you're allowed to touch other on-heap objects that might die in the same GC
cycle. (However, you're not allowed to allocate anything.)

[sof, 2014-05-26 09:41:33]

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/PendingS...
File Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/PendingS...
Source/core/dom/PendingScript.h:113: // PendingScript off heap, along with its
immediate owner.
On 2014/05/26 01:08:25, haraken wrote:
> 
> Hmm, can we probably address the issue using weak processing?
> 
> By calling registerWeakMembers(this, weakCallback), the weak processing is
> called back when the |this| object is going to die. In the weak processing,
> you're allowed to touch other on-heap objects that might die in the same GC
> cycle. (However, you're not allowed to allocate anything.)

If HTMLScriptRunner and HTMLDocumentParser both die at the same time, then no
weak callback gets to run, right? (as that's dependent on the trace() of either
gets to run & register a weak callback.)

If so, then I don't see how this could work in this context.

[Mads Ager (chromium), 2014-05-26 10:46:30]

On 2014/05/26 09:41:33, sof wrote:
>
https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/PendingS...
> File Source/core/dom/PendingScript.h (right):
> 
>
https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/PendingS...
> Source/core/dom/PendingScript.h:113: // PendingScript off heap, along with its
> immediate owner.
> On 2014/05/26 01:08:25, haraken wrote:
> > 
> > Hmm, can we probably address the issue using weak processing?
> > 
> > By calling registerWeakMembers(this, weakCallback), the weak processing is
> > called back when the |this| object is going to die. In the weak processing,
> > you're allowed to touch other on-heap objects that might die in the same GC
> > cycle. (However, you're not allowed to allocate anything.)
> 
> If HTMLScriptRunner and HTMLDocumentParser both die at the same time, then no
> weak callback gets to run, right? (as that's dependent on the trace() of
either
> gets to run & register a weak callback.)
> 
> If so, then I don't see how this could work in this context.

That is correct. Weak processing only happens on objects that are reachable.

[Mads Ager (chromium), 2014-05-26 13:55:36]

Can we move ScriptRunner to the heap, I'm not sure why that one would cause
trouble?

I see the issue with HTMLScriptRunner and friends and I don't see a better
solution right now. We probably need to start looking into Resource and friends
to figure out how to get those on the heap so we can use weak processing for the
resource clients.

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5764: #if ENABLE(OILPAN)
Nit: We are beginning to have multiple ifdef blocks in this oilpan only code.
Maybe we should just ifdef out the entire body of the trace method?

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/PendingS...
File Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/PendingS...
Source/core/dom/PendingScript.h:113: // PendingScript off heap, along with its
immediate owner.
On 2014/05/26 09:41:34, sof wrote:
> On 2014/05/26 01:08:25, haraken wrote:
> > 
> > Hmm, can we probably address the issue using weak processing?
> > 
> > By calling registerWeakMembers(this, weakCallback), the weak processing is
> > called back when the |this| object is going to die. In the weak processing,
> > you're allowed to touch other on-heap objects that might die in the same GC
> > cycle. (However, you're not allowed to allocate anything.)
> 
> If HTMLScriptRunner and HTMLDocumentParser both die at the same time, then no
> weak callback gets to run, right? (as that's dependent on the trace() of
either
> gets to run & register a weak callback.)
> 
> If so, then I don't see how this could work in this context.

Yeah, I see the issue. For now I think we need to keep the OwnPtrs here to make
sure that these owned objects are basically part of the DocumentParser so it can
unregister itself from the pending resources when it dies.

I guess one way of dealing with this longer term would be to get Resource onto
our heap and make the set of clients a weak collection.

The current code is pretty nasty:

DocumentParser
    OwnPtr<HTMLScriptRunner>

~HTMLScriptRunner
    stopWatchingForLoad(...);

void HTMLScriptRunner::stopWatchingForLoad(PendingScript& pendingScript)
{
    ASSERT(pendingScript.watchingForLoad());
    m_host->stopWatchingForLoad(pendingScript.resource());
    pendingScript.setWatchingForLoad(false);
}

We are at the end of the destruction of m_host at this point, but let's call a
method on it! :-)

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.h:44: class ScriptRunner {
I think we could move ScriptRunner to the oilpan heap without issues.
ScriptRunner is owned by the Document and as far as I can tell it is only ever
destructed when the document is going away as well. The only thing it does is
decrement delay-the-load event counts in the document. However, if the document
is going away that shouldn't be necessary (we will not dispatch a load event
when the document is dead anyway)? If that reasoning is correct we should be
able to move this to the heap and have traced Members in both directions and not
update any counts in the destructor here.

[sof, 2014-05-26 21:10:18]

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/PendingS...
File Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/PendingS...
Source/core/dom/PendingScript.h:113: // PendingScript off heap, along with its
immediate owner.
On 2014/05/26 13:55:36, Mads Ager (chromium) wrote:
> On 2014/05/26 09:41:34, sof wrote:
> > On 2014/05/26 01:08:25, haraken wrote:
> > > 
> > > Hmm, can we probably address the issue using weak processing?
> > > 
> > > By calling registerWeakMembers(this, weakCallback), the weak processing is
> > > called back when the |this| object is going to die. In the weak
processing,
> > > you're allowed to touch other on-heap objects that might die in the same
GC
> > > cycle. (However, you're not allowed to allocate anything.)
> > 
> > If HTMLScriptRunner and HTMLDocumentParser both die at the same time, then
no
> > weak callback gets to run, right? (as that's dependent on the trace() of
> either
> > gets to run & register a weak callback.)
> > 
> > If so, then I don't see how this could work in this context.
> 
> Yeah, I see the issue. For now I think we need to keep the OwnPtrs here to
make
> sure that these owned objects are basically part of the DocumentParser so it
can
> unregister itself from the pending resources when it dies.
> 
> I guess one way of dealing with this longer term would be to get Resource onto
> our heap and make the set of clients a weak collection.
> 
> The current code is pretty nasty:
> 
> DocumentParser
>     OwnPtr<HTMLScriptRunner>
> 
> ~HTMLScriptRunner
>     stopWatchingForLoad(...);
> 
> void HTMLScriptRunner::stopWatchingForLoad(PendingScript& pendingScript)
> {
>     ASSERT(pendingScript.watchingForLoad());
>     m_host->stopWatchingForLoad(pendingScript.resource());
>     pendingScript.setWatchingForLoad(false);
> }
> 
> We are at the end of the destruction of m_host at this point, but let's call a
> method on it! :-)
> 

That gave me an idea -- if HTMLScriptRunner is made the ResourceClient instead,
and it is able to handle the finalization without relying on the 'host object',
then we can simplify. The HTMLScriptRunner would then have to redirect the
notifications of finished resource loads to its host, but I think that's an
acceptable arrangement.

[tkent, 2014-05-27 01:04:45]

We need to move the content of ~HTMLScriptRunner to HTMLScriptRunner::detach,
and make sure HTMLDocumentParser::detach is called even in ENABLE(OILPAN).  If
we do so, we can move HTMLScriptRunner to Oilpan heap.

http/tests/misc/write-while-waiting.html is crashing now because
HTMLScriptRunner unregisters its host in ~HTMLScriptRunner. A notification from
Resource is delivered to a detached-but-not-destructed HTMLDocumentParser.

[sof, 2014-05-27 08:30:52]

On 2014/05/27 01:04:45, tkent wrote:
> We need to move the content of ~HTMLScriptRunner to HTMLScriptRunner::detach,
> and make sure HTMLDocumentParser::detach is called even in ENABLE(OILPAN).  If
> we do so, we can move HTMLScriptRunner to Oilpan heap.
> 
> http/tests/misc/write-while-waiting.html is crashing now because
> HTMLScriptRunner unregisters its host in ~HTMLScriptRunner. A notification
from
> Resource is delivered to a detached-but-not-destructed HTMLDocumentParser.

Thanks for explaining the cause; delaying the detach-from-resource-as-a-client
seems harmful to do.

Arranged for detach() to do more work + moved HTMLScriptRunner to the heap along
with making it the ResourceClient; does that mirror what you had in mind?

[sof, 2014-05-27 08:31:03]

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/60001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.h:44: class ScriptRunner {
On 2014/05/26 13:55:36, Mads Ager (chromium) wrote:
> I think we could move ScriptRunner to the oilpan heap without issues.
> ScriptRunner is owned by the Document and as far as I can tell it is only ever
> destructed when the document is going away as well. The only thing it does is
> decrement delay-the-load event counts in the document. However, if the
document
> is going away that shouldn't be necessary (we will not dispatch a load event
> when the document is dead anyway)? If that reasoning is correct we should be
> able to move this to the heap and have traced Members in both directions and
not
> update any counts in the destructor here.

Yes, moving ScriptRunner is doable and easier than its parser counter part
HTMLScriptRunner.

Decrementing those Document counts doesn't seem like a productive operation
Oilpan or not; I emptied the destructor.

[sof, 2014-05-27 08:41:18]

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLDocumentParser.h (left):

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLDocumentParser.h:145: // ResourceClient
Will remove this line.

[tkent, 2014-05-27 12:19:26]

On 2014/05/27 08:30:52, sof wrote:
> On 2014/05/27 01:04:45, tkent wrote:
> > We need to move the content of ~HTMLScriptRunner to
HTMLScriptRunner::detach,
> > and make sure HTMLDocumentParser::detach is called even in ENABLE(OILPAN). 
If
> > we do so, we can move HTMLScriptRunner to Oilpan heap.
> > 
> > http/tests/misc/write-while-waiting.html is crashing now because
> > HTMLScriptRunner unregisters its host in ~HTMLScriptRunner. A notification
> from
> > Resource is delivered to a detached-but-not-destructed HTMLDocumentParser.
> 
> Thanks for explaining the cause; delaying the detach-from-resource-as-a-client
> seems harmful to do.
> 
> Arranged for detach() to do more work + moved HTMLScriptRunner to the heap
along
> with making it the ResourceClient; does that mirror what you had in mind?

Path Set 5 looks ok at this moment.  However it assumes that Resource and
ResourceClient are not garbage-collected.

My intention was to move the content of ~HTMLScriptRunner to
HTMLScriptRunner::detach, and to make ~HTMLScriptRunner empty.

[Mads Ager (chromium), 2014-05-27 12:34:17]

On 2014/05/27 12:19:26, tkent wrote:
> On 2014/05/27 08:30:52, sof wrote:
> > On 2014/05/27 01:04:45, tkent wrote:
> > > We need to move the content of ~HTMLScriptRunner to
> HTMLScriptRunner::detach,
> > > and make sure HTMLDocumentParser::detach is called even in ENABLE(OILPAN).

> If
> > > we do so, we can move HTMLScriptRunner to Oilpan heap.
> > > 
> > > http/tests/misc/write-while-waiting.html is crashing now because
> > > HTMLScriptRunner unregisters its host in ~HTMLScriptRunner. A notification
> > from
> > > Resource is delivered to a detached-but-not-destructed HTMLDocumentParser.
> > 
> > Thanks for explaining the cause; delaying the
detach-from-resource-as-a-client
> > seems harmful to do.
> > 
> > Arranged for detach() to do more work + moved HTMLScriptRunner to the heap
> along
> > with making it the ResourceClient; does that mirror what you had in mind?
> 
> Path Set 5 looks ok at this moment.  However it assumes that Resource and
> ResourceClient are not garbage-collected.
> 
> My intention was to move the content of ~HTMLScriptRunner to
> HTMLScriptRunner::detach, and to make ~HTMLScriptRunner empty.

That might not actually work. We probably do need to unregister as a client from
the Resource even when the Document and the HTMLScript runner die together?

This LGTM from an oilpan perspective. We are moving the code around here. This
code looks like it hasn't been touched in a while. Are there any usual owners of
this part that we should ask to have a look for the non-oilpan version?

[sof, 2014-05-27 12:44:14]

japhet, abarth: in order to make finalization of HTMLScriptRunner objects
possible with Oilpan enabled, HTMLScriptRunner has been turned into a
ResourceClient (rather than the parser) and its 'host' interface had to be
adjusted accordingly. Does that change appear reasonable to you?

[Mads Ager (chromium), 2014-05-27 12:48:59]

Here is a null pointer crash that I just caught in my oilpan build. We have had
an explicit detach call which has not unregistered the DocumentParser as a
client for the resource. We then end up getting notified that the resource is
ready and access the document member which is 0 after detach. This was in an
oilpan build, but I don't see anything about this that is actually oilpan
specific. So, this looks like another reason to make sure to restructure so that
detach removes the DocumentParser as a client from the resource.

#0  0x00007fffea6d0094 in WebCore::Document::haveImportsLoaded (this=0x0)
    at ../../third_party/WebKit/Source/core/dom/Document.cpp:846
#1  0x00007fffe951bade in WebCore::Document::isRenderingReady (this=0x0)
    at ../../third_party/WebKit/Source/core/dom/Document.h:430
#2  0x00007fffeaab9f5e in WebCore::Document::isScriptExecutionReady (this=0x0)
    at ../../third_party/WebKit/Source/core/dom/Document.h:431
#3  0x00007fffeaab8c27 in WebCore::HTMLScriptRunner::isPendingScriptReady (
    this=0x6d95ff149e8, script=...)
    at ../../third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:106
#4  0x00007fffeaab93f7 in
WebCore::HTMLScriptRunner::executeParsingBlockingScripts (this=0x6d95ff149e8)
    at ../../third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:204
#5  0x00007fffeaab956e in
WebCore::HTMLScriptRunner::executeScriptsWaitingForLoad (this=0x6d95ff149e8,
resource=0x6d95ffaa310)
    at ../../third_party/WebKit/Source/core/html/parser/HTMLScriptRunner.cpp:214
#6  0x00007fffeaa9ff65 in WebCore::HTMLDocumentParser::notifyFinished (
    this=0x7fffc7cb8f08, cachedResource=0x6d95ffaa310)
    at
../../third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:991
#7  0x00007fffeade124c in WebCore::Resource::checkNotify (this=0x6d95ffaa310)
    at ../../third_party/WebKit/Source/core/fetch/Resource.cpp:197
#8  0x00007fffeade1766 in WebCore::Resource::finishOnePart (this=0x6d95ffaa310)
---Type <return> to continue, or q <return> to quit---
    at ../../third_party/WebKit/Source/core/fetch/Resource.cpp:249
#9  0x00007fffeade1837 in WebCore::Resource::finish (this=0x6d95ffaa310, 
    finishTime=1751119.3137020001)
    at ../../third_party/WebKit/Source/core/fetch/Resource.cpp:257
#10 0x00007fffeae044e5 in WebCore::ResourceLoader::didFinishLoading (
    this=0x6d95ff8c910, finishTime=1751119.3137020001, encodedDataLength=382)
    at ../../third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:427
#11 0x00007fffefbc4ca8 in content::WebURLLoaderImpl::Context::OnCompletedRequest
(this=0x363e4b558b60, error_code=0, was_ignored_by_handler=false, 
    stale_copy_in_cache=false, security_info="", completion_time=..., 
    total_transfer_size=382) at ../../content/child/web_url_loader_impl.cc:639
#12 0x00007fffefb95197 in content::ResourceDispatcher::OnRequestComplete (
    this=0x363e4b3b1390, request_id=535, request_complete_data=...)
    at ../../content/child/resource_dispatcher.cc:544
#13 0x00007fffefb9935a in DispatchToMethod<content::ResourceDispatcher, void
(content::ResourceDispatcher::*)(int, ResourceMsg_RequestCompleteData const&),
int, ResourceMsg_RequestCompleteData> (obj=0x363e4b3b1390, method=
    (void (content::ResourceDispatcher::*)(content::ResourceDispatcher * const,
int, const ResourceMsg_RequestCompleteData &)) 0x7fffefb94f60
<content::ResourceDispatcher::OnRequestComplete(int,
ResourceMsg_RequestCompleteData const&)>, 
    arg=...) at ../../base/tuple.h:555
#14 0x00007fffefb97e84 in
ResourceMsg_RequestComplete::Dispatch<content::ResourceDispatcher,
content::ResourceDispatcher, void, void (content::ResourceDispatcher::*)(int,
ResourceMsg_RequestCompleteData const&)> (msg=0x363e4e45cdc8, 
    obj=0x363e4b3b1390, sender=0x363e4b3b1390, parameter=0x0, func=
---Type <return> to continue, or q <return> to quit---
    (void (content::ResourceDispatcher::*)(content::ResourceDispatcher * const,
int, const ResourceMsg_RequestCompleteData &)) 0x7fffefb94f60
<content::ResourceDispatcher::OnRequestComplete(int,
ResourceMsg_RequestCompleteData const&)>)
    at ../../content/common/resource_messages.h:301
#15 0x00007fffefb9619c in content::ResourceDispatcher::DispatchMessage (
    this=0x363e4b3b1390, message=...)
    at ../../content/child/resource_dispatcher.cc:665
#16 0x00007fffefb93d79 in content::ResourceDispatcher::OnMessageReceived (
    this=0x363e4b3b1390, message=...)
    at ../../content/child/resource_dispatcher.cc:308
#17 0x00007fffefae0870 in content::ChildThread::OnMessageReceived (
    this=0x363e4b266da8, msg=...) at ../../content/child/child_thread.cc:422
#18 0x00007ffff480ff6e in IPC::ChannelProxy::Context::OnDispatchMessage (
    this=0x363e4b2d1b60, message=...) at ../../ipc/ipc_channel_proxy.cc:273
#19 0x00007ffff4815432 in base::internal::RunnableAdapter<void
(IPC::ChannelProxy::Context::*)(IPC::Message const&)>::Run (this=0x7fffcc9df1f0,

    object=0x363e4b2d1b60, a1=...) at ../../base/bind_internal.h:190
#20 0x00007ffff4814a30 in base::internal::InvokeHelper<false, void,
base::internal::RunnableAdapter<void
(IPC::ChannelProxy::Context::*)(IPC::Message const&)>, void
(IPC::ChannelProxy::Context* const&, IPC::Message
const&)>::MakeItSo(base::internal::RunnableAdapter<void
(IPC::ChannelProxy::Context::*)(IPC::Message const&)>,
IPC::ChannelProxy::Context* const&, IPC::Message const&) (runnable=..., 
    a1=@0x363e4e45cdc0: 0x363e4b2d1b60, a2=...)
    at ../../base/bind_internal.h:898

[sof, 2014-05-27 13:03:18]

On 2014/05/27 12:48:59, Mads Ager (chromium) wrote:
> Here is a null pointer crash that I just caught in my oilpan build. We have
had
> an explicit detach call which has not unregistered the DocumentParser as a
> client for the resource. We then end up getting notified that the resource is
> ready and access the document member which is 0 after detach. This was in an
> oilpan build, but I don't see anything about this that is actually oilpan
> specific. So, this looks like another reason to make sure to restructure so
that
> detach removes the DocumentParser as a client from the resource.
> 

It seems like a reasonable shift in responsibility. I've only seen that crash
reproduce in an Oilpan setting, and as we do always detach the parser (and
script runner) in non-Oilpan builds, these late notifications will not manifest
there?

tkent's https://codereview.chromium.org/298863010/#msg7 describes this crash
condition also.

[tkent, 2014-05-27 23:31:49]

+eseidel, morrita

We have a similar crash without Oilpan. crbug.com/376143.

[sof, 2014-05-28 08:16:47]

tkent: are you ok with moving ahead with this CL?

[tkent, 2014-05-28 22:18:26]

On 2014/05/28 08:16:47, sof wrote:
> tkent: are you ok with moving ahead with this CL?

It's ok.
Anyway, this change needs review by an HTML parser expert (eseidel or abarth?).

[eseidel, 2014-05-28 22:43:38]

lgtm

I'm not sure I have all this paged back into my mind, but what you've done here
looks sane.  I'm really not sure why this code ended up this complicated in the
first place.  The idea of pending scripts is not a complicated one....

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunner.cpp (left):

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.cpp:46: for (size_t i = 0; i <
m_scriptsToExecuteSoon.size(); ++i)
Wow this code is scary.

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.cpp:61: detach();
Isn't this a bug in ~Document()?

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.cpp:70: if (m_document) {
Early return is better.

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.cpp:75: PendingScript pendingScript =
m_scriptsToExecuteAfterParsing.takeFirst();
Why can't PendingScript just do this in its destructor?

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.cpp:177:
pendingScript.resource()->addClient(this);
Bleh.  Manual add/remove calls are nasty.  They're just bugs waiting to happen.

[haraken, 2014-05-29 01:46:08]

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/PendingS...
File Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/PendingS...
Source/core/dom/PendingScript.h:48: ALLOW_ONLY_INLINE_ALLOCATION();

I think you need to add WTF_ALLOW_INIT_WITH_MEM_FUNCTION (which specifies
canInitializeWithMemset). I wonder why this CL compiles without setting
canInitializeWithMemset, but probably is it not needed for off-heap Vectors? (In
this CL, PendingScript is used in off-heap Vectors, not on-heap Vectors.)

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunner.cpp (left):

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.cpp:51: m_document->decrementLoadEventDelayCount();

Just help me understand: I agree that we no longer need this code in oilpan
builds because the Document and the ScriptRunner die together, but why can we
remove the code in non-oilpan builds?

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.h:70: // PendingScript does have a (trivial)
destructor, however.

Just help me understand: What prevents us from making these collections on-heap
in this CL?

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.h:73: HashMap<ScriptLoader*, PendingScript>
m_pendingAsyncScripts;

The raw pointers in the key looks safe, since they are explicitly cleared in
notifyScriptReady/notifyScriptLoadError.

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.cpp:61: detach();
On 2014/05/28 22:43:38, eseidel wrote:
> Isn't this a bug in ~Document()?

Agreed, I guess the Document should always call explicit detach() before being
destructed.

(In general, it's not a good thing to call explicit detach() in a destructor,
since the timing when the destructor is called is not deterministic.)

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.cpp:70: if (m_document) {

Does this check mean that detach() can be called twice?

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.cpp:75: PendingScript pendingScript =
m_scriptsToExecuteAfterParsing.takeFirst();
On 2014/05/28 22:43:38, eseidel wrote:
> Why can't PendingScript just do this in its destructor?

That's a good question.

I think that in this CL we can just call this in the destructor, but I don't
think it's a right fix. I guess a right fix would be:

- Make the Document always call explicit HTMLScriptRunner::detach() before the
HTMLScriptRunner destructed.

- Make m_scriptsToExecuteAfterParsing a HeapDeque.

- Make the destructor empty.

What do you think?

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.h:100: HTMLScriptRunnerHost* m_host;

This raw pointer looks OK, but it'd be better if we could make it a Member.

The only derived class of HTMLScriptRunnerHost is HTMLDocumentParser, and
HTMLDocumentParser is already on heap.

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.h:102: Deque<PendingScript>
m_scriptsToExecuteAfterParsing; //
http://www.whatwg.org/specs/web-apps/current-work/#list-of-scripts-that-will-...

Add a FIXME to change this to a HeapDeque.

[sof, 2014-05-29 06:02:06]

Thanks for the reviews. I hope I've managed to clarify some issues and problems
when having to correctly finalize in this mixed-mode setting of GCed objects
(script runners) and not (resources).

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/PendingS...
File Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/PendingS...
Source/core/dom/PendingScript.h:48: ALLOW_ONLY_INLINE_ALLOCATION();
On 2014/05/29 01:46:08, haraken wrote:
> 
> I think you need to add WTF_ALLOW_INIT_WITH_MEM_FUNCTION (which specifies
> canInitializeWithMemset). I wonder why this CL compiles without setting
> canInitializeWithMemset, but probably is it not needed for off-heap Vectors?
(In
> this CL, PendingScript is used in off-heap Vectors, not on-heap Vectors.)

Yes, PendingScript appears as a part object, in off-heap vectors and deques here
for a good reason. i.e., the owning/controlling script runner objects can then
safely access them when they're finalized.

Vector<> is not a garbage collected object, so it doesn't consider
canInitializeWithMemset. See the Vector.h COMPILE_ASSERT()s.

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunner.cpp (left):

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.cpp:51: m_document->decrementLoadEventDelayCount();
On 2014/05/29 01:46:08, haraken wrote:
> 
> Just help me understand: I agree that we no longer need this code in oilpan
> builds because the Document and the ScriptRunner die together, but why can we
> remove the code in non-oilpan builds?

For the same reason, essentially. The ScriptRunner is OwnPtr'ed by the Document
and only released on Document destruct. Decrementing counts and attempting to
possibly schedule a future load event using a near-dead Document isn't
productive work. I didn't see the value of keeping it.

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.h:70: // PendingScript does have a (trivial)
destructor, however.
On 2014/05/29 01:46:08, haraken wrote:
> 
> Just help me understand: What prevents us from making these collections
on-heap
> in this CL?

This goes to the core of this CL. If they move to the heap, we have a
finalization problem for PendingScript, as it cannot access its runner object
which is the script resource's ResourceClient. The runner object also being on
the heap.

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.cpp:61: detach();
On 2014/05/29 01:46:08, haraken wrote:
> On 2014/05/28 22:43:38, eseidel wrote:
> > Isn't this a bug in ~Document()?
> 
> Agreed, I guess the Document should always call explicit detach() before being
> destructed.
> 
> (In general, it's not a good thing to call explicit detach() in a destructor,
> since the timing when the destructor is called is not deterministic.)

I've tried to improve the comment to clarify. If the Document, parser and script
runner object die in the same GC, no explicit detach will happen.

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.cpp:70: if (m_document) {
On 2014/05/29 01:46:08, haraken wrote:
> 
> Does this check mean that detach() can be called twice?

Yes it does. How would you distinguish between an explicitly detached script
runner object that's later finalized and one that's finalized along with its
parser (hence no detach() happens)?

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.cpp:70: if (m_document) {
On 2014/05/28 22:43:38, eseidel wrote:
> Early return is better.

Thanks, done.

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.cpp:75: PendingScript pendingScript =
m_scriptsToExecuteAfterParsing.takeFirst();
On 2014/05/29 01:46:08, haraken wrote:
> On 2014/05/28 22:43:38, eseidel wrote:
> > Why can't PendingScript just do this in its destructor?
> 
> That's a good question.
> 
> I think that in this CL we can just call this in the destructor, but I don't
> think it's a right fix. I guess a right fix would be:
> 
> - Make the Document always call explicit HTMLScriptRunner::detach() before the
> HTMLScriptRunner destructed.
> 

Can't ever guarantee the "always" bit, destruction may happen without explicit
detachment.

> - Make m_scriptsToExecuteAfterParsing a HeapDeque.
> 

How would you refer to the HTMLScriptRunner object from the objects that heap
deque contains?

> - Make the destructor empty.
> 

Not possible given the above.

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.h:100: HTMLScriptRunnerHost* m_host;
On 2014/05/29 01:46:08, haraken wrote:
> 
> This raw pointer looks OK, but it'd be better if we could make it a Member.
> 
> The only derived class of HTMLScriptRunnerHost is HTMLDocumentParser, and
> HTMLDocumentParser is already on heap.
> 

Yes, it is safe.

https://codereview.chromium.org/298863010/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLScriptRunner.h:102: Deque<PendingScript>
m_scriptsToExecuteAfterParsing; //
http://www.whatwg.org/specs/web-apps/current-work/#list-of-scripts-that-will-...
On 2014/05/29 01:46:08, haraken wrote:
> 
> Add a FIXME to change this to a HeapDeque.

It's a non-heap version for a reason. i.e., we cannot finalize that sequence
with the help of this HTMLScriptRunner object if it lives on the heap. So, no
FIXME.

[haraken, 2014-05-29 11:32:07]

LGTM with some questions.

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.h:70: // PendingScript does have a (trivial)
destructor, however.
On 2014/05/29 06:02:07, sof wrote:
> On 2014/05/29 01:46:08, haraken wrote:
> > 
> > Just help me understand: What prevents us from making these collections
> on-heap
> > in this CL?
> 
> This goes to the core of this CL. If they move to the heap, we have a
> finalization problem for PendingScript, as it cannot access its runner object
> which is the script resource's ResourceClient. The runner object also being on
> the heap.

Just help me understand: Where does ~PendingScript touch ResourceClient? I
failed to find the code...

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:61: // and this script runner
object are swept out in the same GC.

Understood. Then I get back to the eseidel's question: why can't the detach()
code just stay in the destructor?

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
File Source/core/html/parser/HTMLScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.h:100: HTMLScriptRunnerHost* m_host;

Even if this raw pointer is safe, I'd prefer making it a Member since it makes
the lifetime relationship clearer :)

[sof, 2014-05-29 20:39:54]

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.h:70: // PendingScript does have a (trivial)
destructor, however.
On 2014/05/29 11:32:08, haraken wrote:
> On 2014/05/29 06:02:07, sof wrote:
> > On 2014/05/29 01:46:08, haraken wrote:
> > > 
> > > Just help me understand: What prevents us from making these collections
> > on-heap
> > > in this CL?
> > 
> > This goes to the core of this CL. If they move to the heap, we have a
> > finalization problem for PendingScript, as it cannot access its runner
object
> > which is the script resource's ResourceClient. The runner object also being
on
> > the heap.
> 
> Just help me understand: Where does ~PendingScript touch ResourceClient? I
> failed to find the code...

If you look at ResourceOwner<>::setResource() (PendingScript derives from
ResourceOwner), you may find it there.

Part of detaching/deleting a ResourceClient is to remove itself as a client from
the resources it has added itself to. If ~PendingScript were on the heap and
HTMLScriptRunner also, the finalization problem would manifest itself from
~PendingScript. That's what I referred to in the above/previous comment.

Does that help explaining?

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:61: // and this script runner
object are swept out in the same GC.
On 2014/05/29 11:32:08, haraken wrote:
> 
> Understood. Then I get back to the eseidel's question: why can't the detach()
> code just stay in the destructor?

We have to have Document::detachParser() behave as synchronously as in a
non-Oilpan setting -- it currently calls detach on the parser, followed by
deleting it (non-Oilpan.) Parser detaching happens either from the Document dtor
or when cancelling the parser for other reasons.

If we were to delay stopping these pending scripts until the next time the
Oilpan GC runs (and ~HTMLScriptRunner is invoked when sweeping), that would just
be wrong. Risking either scripts to complete loading and touching the dead or
detached parser.

That's why we have moved this dtor code into detach(). The only wrinkle with
that is if we GC the Document and parser at the same time. We cannot detach the
parser from the Document finalizer, as the parser is another heap object, so we
have to arrange for the effects of that detach to happen by other ways for this
HTMLScriptRunner. Which is done by calling detach() from its
destructor/finalizer.

Does that help explaining why? 

See also tkent's comment on why this is needed:
https://codereview.chromium.org/298863010/#msg7
Similarly ager's: https://codereview.chromium.org/298863010/#msg14

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
File Source/core/html/parser/HTMLScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.h:100: HTMLScriptRunnerHost* m_host;
On 2014/05/29 11:32:08, haraken wrote:
> 
> Even if this raw pointer is safe, I'd prefer making it a Member since it
makes
> the lifetime relationship clearer :)

I will (have to) extend HTMLScriptRunnerHost to accommodate that suggestion.

[haraken, 2014-05-30 01:28:50]

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.h:70: // PendingScript does have a (trivial)
destructor, however.
On 2014/05/29 20:39:54, sof wrote:
> On 2014/05/29 11:32:08, haraken wrote:
> > On 2014/05/29 06:02:07, sof wrote:
> > > On 2014/05/29 01:46:08, haraken wrote:
> > > > 
> > > > Just help me understand: What prevents us from making these collections
> > > on-heap
> > > > in this CL?
> > > 
> > > This goes to the core of this CL. If they move to the heap, we have a
> > > finalization problem for PendingScript, as it cannot access its runner
> object
> > > which is the script resource's ResourceClient. The runner object also
being
> on
> > > the heap.
> > 
> > Just help me understand: Where does ~PendingScript touch ResourceClient? I
> > failed to find the code...
> 
> If you look at ResourceOwner<>::setResource() (PendingScript derives from
> ResourceOwner), you may find it there.
> 
> Part of detaching/deleting a ResourceClient is to remove itself as a client
from
> the resources it has added itself to. If ~PendingScript were on the heap and
> HTMLScriptRunner also, the finalization problem would manifest itself from
> ~PendingScript. That's what I referred to in the above/previous comment.
> 
> Does that help explaining?

I understand your point, but my question is:

If we move PendingScript to the heap and have the PendingScript and the
ResourceClient hold strong members to each other, we can make the PendingScript
and the ResourceClient die together. Then ResourceClient/PendingScript no longer
needs to touch PendingScript/ResourceClient in its destructor and thus the
touch-other-on-heap-objects-in-destructor issue is gone, isn't it?

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:61: // and this script runner
object are swept out in the same GC.
On 2014/05/29 20:39:54, sof wrote:
> On 2014/05/29 11:32:08, haraken wrote:
> > 
> > Understood. Then I get back to the eseidel's question: why can't the
detach()
> > code just stay in the destructor?
> 
> We have to have Document::detachParser() behave as synchronously as in a
> non-Oilpan setting -- it currently calls detach on the parser, followed by
> deleting it (non-Oilpan.) Parser detaching happens either from the Document
dtor
> or when cancelling the parser for other reasons.
> 
> If we were to delay stopping these pending scripts until the next time the
> Oilpan GC runs (and ~HTMLScriptRunner is invoked when sweeping), that would
just
> be wrong. Risking either scripts to complete loading and touching the dead or
> detached parser.
> 
> That's why we have moved this dtor code into detach(). The only wrinkle with
> that is if we GC the Document and parser at the same time. We cannot detach
the
> parser from the Document finalizer, as the parser is another heap object, so
we
> have to arrange for the effects of that detach to happen by other ways for
this
> HTMLScriptRunner. Which is done by calling detach() from its
> destructor/finalizer.
> 
> Does that help explaining why? 
> 
> See also tkent's comment on why this is needed:
> https://codereview.chromium.org/298863010/#msg7
> Similarly ager's: https://codereview.chromium.org/298863010/#msg14

Makes sense, sorry I was behind what you've already discussed.

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
File Source/core/html/parser/HTMLScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.h:100: HTMLScriptRunnerHost* m_host;
On 2014/05/29 20:39:54, sof wrote:
> On 2014/05/29 11:32:08, haraken wrote:
> > 
> > Even if this raw pointer is safe, I'd prefer making it a Member since it
makes
> > the lifetime relationship clearer :)
> 
> I will (have to) extend HTMLScriptRunnerHost to accommodate that suggestion.

A follow-up is fine :)

[sof, 2014-05-30 06:26:39]

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.h:70: // PendingScript does have a (trivial)
destructor, however.
On 2014/05/30 01:28:50, haraken wrote:
> On 2014/05/29 20:39:54, sof wrote:
> > On 2014/05/29 11:32:08, haraken wrote:
> > > On 2014/05/29 06:02:07, sof wrote:
> > > > On 2014/05/29 01:46:08, haraken wrote:
> > > > > 
> > > > > Just help me understand: What prevents us from making these
collections
> > > > on-heap
> > > > > in this CL?
> > > > 
> > > > This goes to the core of this CL. If they move to the heap, we have a
> > > > finalization problem for PendingScript, as it cannot access its runner
> > object
> > > > which is the script resource's ResourceClient. The runner object also
> being
> > on
> > > > the heap.
> > > 
> > > Just help me understand: Where does ~PendingScript touch ResourceClient?
I
> > > failed to find the code...
> > 
> > If you look at ResourceOwner<>::setResource() (PendingScript derives from
> > ResourceOwner), you may find it there.
> > 
> > Part of detaching/deleting a ResourceClient is to remove itself as a client
> from
> > the resources it has added itself to. If ~PendingScript were on the heap
and
> > HTMLScriptRunner also, the finalization problem would manifest itself from
> > ~PendingScript. That's what I referred to in the above/previous comment.
> > 
> > Does that help explaining?
> 
> I understand your point, but my question is:
> 
> If we move PendingScript to the heap and have the PendingScript and the
> ResourceClient hold strong members to each other, we can make the
PendingScript
> and the ResourceClient die together. Then ResourceClient/PendingScript no
longer
> needs to touch PendingScript/ResourceClient in its destructor and thus the
> touch-other-on-heap-objects-in-destructor issue is gone, isn't it?

I just don't see how to correctly address the ResourceClient removing itself as
the client of the PendingScript's resource during finalization with that
arrangement.

They die together now and resources can be safely unregistered then by virtue of
the PendingScripts being part (or inline) objects of the script runner objects
(which are on the heap.)

[haraken, 2014-05-30 06:49:41]

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.h:70: // PendingScript does have a (trivial)
destructor, however.
On 2014/05/30 06:26:40, sof wrote:
> On 2014/05/30 01:28:50, haraken wrote:
> > On 2014/05/29 20:39:54, sof wrote:
> > > On 2014/05/29 11:32:08, haraken wrote:
> > > > On 2014/05/29 06:02:07, sof wrote:
> > > > > On 2014/05/29 01:46:08, haraken wrote:
> > > > > > 
> > > > > > Just help me understand: What prevents us from making these
> collections
> > > > > on-heap
> > > > > > in this CL?
> > > > > 
> > > > > This goes to the core of this CL. If they move to the heap, we have a
> > > > > finalization problem for PendingScript, as it cannot access its runner
> > > object
> > > > > which is the script resource's ResourceClient. The runner object also
> > being
> > > on
> > > > > the heap.
> > > > 
> > > > Just help me understand: Where does ~PendingScript touch ResourceClient?
I
> > > > failed to find the code...
> > > 
> > > If you look at ResourceOwner<>::setResource() (PendingScript derives from
> > > ResourceOwner), you may find it there.
> > > 
> > > Part of detaching/deleting a ResourceClient is to remove itself as a
client
> > from
> > > the resources it has added itself to. If ~PendingScript were on the heap
and
> > > HTMLScriptRunner also, the finalization problem would manifest itself from
> > > ~PendingScript. That's what I referred to in the above/previous comment.
> > > 
> > > Does that help explaining?
> > 
> > I understand your point, but my question is:
> > 
> > If we move PendingScript to the heap and have the PendingScript and the
> > ResourceClient hold strong members to each other, we can make the
> PendingScript
> > and the ResourceClient die together. Then ResourceClient/PendingScript no
> longer
> > needs to touch PendingScript/ResourceClient in its destructor and thus the
> > touch-other-on-heap-objects-in-destructor issue is gone, isn't it?
> 
> I just don't see how to correctly address the ResourceClient removing itself
as
> the client of the PendingScript's resource during finalization with that
> arrangement.
> 
> They die together now and resources can be safely unregistered then by virtue
of
> the PendingScripts being part (or inline) objects of the script runner objects
> (which are on the heap.)

Understood. I'm just wondering if we can eliminate the need of unregistering the
client by moving both client and PendingScript to the heap (then the
finalization issue will be gone because the client and the PendingScript die
together and thus we don't need to do the unregistration when the client dies).

Either way, I'm completely fine with the current CL. Mads and Eric are fine, so
I think you can go ahead and land. Sorry about interrupting you a lot.

[sof, 2014-05-30 09:43:39]

Thanks everyone; will try to land this now.

Oilpan bots are temporarily red in unit tests with ToT; will verify local
results for those w/ final patchset once that clears up.

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/80001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunner.h:70: // PendingScript does have a (trivial)
destructor, however.
On 2014/05/30 06:49:42, haraken wrote:
> On 2014/05/30 06:26:40, sof wrote:
> > On 2014/05/30 01:28:50, haraken wrote:
> > > On 2014/05/29 20:39:54, sof wrote:
> > > > On 2014/05/29 11:32:08, haraken wrote:
> > > > > On 2014/05/29 06:02:07, sof wrote:
> > > > > > On 2014/05/29 01:46:08, haraken wrote:
> > > > > > > 
> > > > > > > Just help me understand: What prevents us from making these
> > collections
> > > > > > on-heap
> > > > > > > in this CL?
> > > > > > 
> > > > > > This goes to the core of this CL. If they move to the heap, we have
a
> > > > > > finalization problem for PendingScript, as it cannot access its
runner
> > > > object
> > > > > > which is the script resource's ResourceClient. The runner object
also
> > > being
> > > > on
> > > > > > the heap.
> > > > > 
> > > > > Just help me understand: Where does ~PendingScript touch
ResourceClient?
> I
> > > > > failed to find the code...
> > > > 
> > > > If you look at ResourceOwner<>::setResource() (PendingScript derives
from
> > > > ResourceOwner), you may find it there.
> > > > 
> > > > Part of detaching/deleting a ResourceClient is to remove itself as a
> client
> > > from
> > > > the resources it has added itself to. If ~PendingScript were on the
heap
> and
> > > > HTMLScriptRunner also, the finalization problem would manifest itself
from
> > > > ~PendingScript. That's what I referred to in the above/previous
comment.
> > > > 
> > > > Does that help explaining?
> > > 
> > > I understand your point, but my question is:
> > > 
> > > If we move PendingScript to the heap and have the PendingScript and the
> > > ResourceClient hold strong members to each other, we can make the
> > PendingScript
> > > and the ResourceClient die together. Then ResourceClient/PendingScript no
> > longer
> > > needs to touch PendingScript/ResourceClient in its destructor and thus
the
> > > touch-other-on-heap-objects-in-destructor issue is gone, isn't it?
> > 
> > I just don't see how to correctly address the ResourceClient removing
itself
> as
> > the client of the PendingScript's resource during finalization with that
> > arrangement.
> > 
> > They die together now and resources can be safely unregistered then by
virtue
> of
> > the PendingScripts being part (or inline) objects of the script runner
objects
> > (which are on the heap.)
> 
> Understood. I'm just wondering if we can eliminate the need of unregistering
the
> client by moving both client and PendingScript to the heap (then the
> finalization issue will be gone because the client and the PendingScript die
> together and thus we don't need to do the unregistration when the client
dies).
> 

The problem is that PendingScript contains a Resource that will have to be
notified when the client dies. Unless the Resource client registration mechanism
is rethought/reconsidered (or Resource moves to the heap), then I just don't see
how this can work.

> Either way, I'm completely fine with the current CL. Mads and Eric are fine,
so
> I think you can go ahead and land. Sorry about interrupting you a lot.

It makes for a better outcome, so I don't mind one bit really. Thanks :)

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
File Source/core/html/parser/HTMLScriptRunner.h (right):

https://codereview.chromium.org/298863010/diff/100001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.h:100: HTMLScriptRunnerHost* m_host;
On 2014/05/30 01:28:51, haraken wrote:
> On 2014/05/29 20:39:54, sof wrote:
> > On 2014/05/29 11:32:08, haraken wrote:
> > > 
> > > Even if this raw pointer is safe, I'd prefer making it a Member since it
> makes
> > > the lifetime relationship clearer :)
> > 
> > I will (have to) extend HTMLScriptRunnerHost to accommodate that
suggestion.
> 
> A follow-up is fine :)

Done now

[sof, 2014-05-30 09:43:51]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-05-30 09:44:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/298863010/120001

[commit-bot: I haz the power, 2014-05-30 13:36:00]

Change committed as 175127