Oilpan: Disable lazy seeping for ImageLoaders

Oilpan: Disable lazy seeping for ImageLoaders

We must run the destructor in the eager sweeping phase and call                                    
m_image->removeClient(this). Otherwise, the ImageResource can invoke                               
didAddClient() for the ImageLoader that is about to die in the current                             
lazy sweeping, and the didAddClient() can access on-heap objects that                              
have already been finalized in the current lazy sweeping.

BUG=

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

[haraken, 2015-06-09 11:03:46]

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

[haraken, 2015-06-09 11:03:47]

As commented in https://codereview.chromium.org/1151163002/#msg13,
Heap::willObjectBeLazilySwept can return a wrong result in some edge cases. I
think ImageLoader is hitting the cases. This CL removes
Heap::willObjectBeLazilySwept from ImageLoader and avoids that problem.

I confirmed that the crash in dom-modify is gone.

PTAL.

[sof, 2015-06-09 11:06:38]

On 2015/06/09 11:03:47, haraken wrote:
> As commented in https://codereview.chromium.org/1151163002/#msg13,
> Heap::willObjectBeLazilySwept can return a wrong result in some edge cases. I
> think ImageLoader is hitting the cases. This CL removes
> Heap::willObjectBeLazilySwept from ImageLoader and avoids that problem.
> 
> I confirmed that the crash in dom-modify is gone.
> 
> PTAL.

This cannot be the reason for it though, as we cannot run tasks while a page is
being swept?

[haraken, 2015-06-09 11:13:30]

On 2015/06/09 11:06:38, sof wrote:
> On 2015/06/09 11:03:47, haraken wrote:
> > As commented in https://codereview.chromium.org/1151163002/#msg13,
> > Heap::willObjectBeLazilySwept can return a wrong result in some edge cases.
I
> > think ImageLoader is hitting the cases. This CL removes
> > Heap::willObjectBeLazilySwept from ImageLoader and avoids that problem.
> > 
> > I confirmed that the crash in dom-modify is gone.
> > 
> > PTAL.
> 
> This cannot be the reason for it though, as we cannot run tasks while a page
is
> being swept?

Yeah, agreed.

I was thinking that one theoretical possibility is that some destructor executes
JS and a micro task is scheduled after the JS execution. However, that cannot
happen because we are in ScriptForbiddenScope while sweeping objects.

Although it seems true that this CL fixes the crash (as far as I run dom-modify
10 times), we need to consider a bit more to figure out what's going on here.

[sof, 2015-06-09 11:18:15]

On 2015/06/09 11:13:30, haraken wrote:
> On 2015/06/09 11:06:38, sof wrote:
> > On 2015/06/09 11:03:47, haraken wrote:
> > > As commented in https://codereview.chromium.org/1151163002/#msg13,
> > > Heap::willObjectBeLazilySwept can return a wrong result in some edge
cases.
> I
> > > think ImageLoader is hitting the cases. This CL removes
> > > Heap::willObjectBeLazilySwept from ImageLoader and avoids that problem.
> > > 
> > > I confirmed that the crash in dom-modify is gone.
> > > 
> > > PTAL.
> > 
> > This cannot be the reason for it though, as we cannot run tasks while a page
> is
> > being swept?
> 
> Yeah, agreed.
> 
> I was thinking that one theoretical possibility is that some destructor
executes
> JS and a micro task is scheduled after the JS execution. However, that cannot
> happen because we are in ScriptForbiddenScope while sweeping objects.
> 
> Although it seems true that this CL fixes the crash (as far as I run
dom-modify
> 10 times), we need to consider a bit more to figure out what's going on here.

I don't mind switching as eager finalization is now preferred over is*Swept(),
lgtm, but I don't see&understand the bug.

[haraken, 2015-06-09 11:53:18]

I haven't yet figured out what's going on, but two comments.

https://codereview.chromium.org/1174463003/diff/1/Source/core/loader/ImageLoa...
File Source/core/loader/ImageLoader.h (right):

https://codereview.chromium.org/1174463003/diff/1/Source/core/loader/ImageLoa...
Source/core/loader/ImageLoader.h:168: ~ImageLoaderClientRemover();

Question: Who guarantees that m_loader is still alive when
~ImageLoaderClientRemover() runs?

https://codereview.chromium.org/1174463003/diff/1/Source/core/loader/ImageLoa...
Source/core/loader/ImageLoader.h:175: // Oilpan: This ImageLoader object must
outlive its clients because they

Now we have an eager sweep heap, but it seems not easy to fix this hack. To
remove the ImageLoaderClientRemover, we need to let ImageBitmap's destructor
touch the ImageLoader. However, we cannot achieve it by moving ImageBitmap to
the eager sweep heap, because the ImageLoader is already on the eager sweep
heap.

[haraken, 2015-06-09 12:14:45]

Identified the root cause :) See the CL description.

[haraken, 2015-06-09 12:14:49]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-06-09 12:14:49]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1174463003/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-06-09 12:15:41]

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

[sof, 2015-06-09 12:16:58]

On 2015/06/09 11:18:15, sof wrote:
> On 2015/06/09 11:13:30, haraken wrote:
> > On 2015/06/09 11:06:38, sof wrote:
> > > On 2015/06/09 11:03:47, haraken wrote:
> > > > As commented in https://codereview.chromium.org/1151163002/#msg13,
> > > > Heap::willObjectBeLazilySwept can return a wrong result in some edge
> cases.
> > I
> > > > think ImageLoader is hitting the cases. This CL removes
> > > > Heap::willObjectBeLazilySwept from ImageLoader and avoids that problem.
> > > > 
> > > > I confirmed that the crash in dom-modify is gone.
> > > > 
> > > > PTAL.
> > > 
> > > This cannot be the reason for it though, as we cannot run tasks while a
page
> > is
> > > being swept?
> > 
> > Yeah, agreed.
> > 
> > I was thinking that one theoretical possibility is that some destructor
> executes
> > JS and a micro task is scheduled after the JS execution. However, that
cannot
> > happen because we are in ScriptForbiddenScope while sweeping objects.
> > 
> > Although it seems true that this CL fixes the crash (as far as I run
> dom-modify
> > 10 times), we need to consider a bit more to figure out what's going on
here.
> 
> I don't mind switching as eager finalization is now preferred over is*Swept(),
> lgtm, but I don't see&understand the bug.

I see this ImageLoader task being run() with DOMTimer::fired() further up the
stack. It is unsafe unless you're based on a tree having
https://codereview.chromium.org/1160123004/ (or newer.)

[haraken, 2015-06-09 12:20:48]

On 2015/06/09 12:16:58, sof wrote:
> On 2015/06/09 11:18:15, sof wrote:
> > On 2015/06/09 11:13:30, haraken wrote:
> > > On 2015/06/09 11:06:38, sof wrote:
> > > > On 2015/06/09 11:03:47, haraken wrote:
> > > > > As commented in https://codereview.chromium.org/1151163002/#msg13,
> > > > > Heap::willObjectBeLazilySwept can return a wrong result in some edge
> > cases.
> > > I
> > > > > think ImageLoader is hitting the cases. This CL removes
> > > > > Heap::willObjectBeLazilySwept from ImageLoader and avoids that
problem.
> > > > > 
> > > > > I confirmed that the crash in dom-modify is gone.
> > > > > 
> > > > > PTAL.
> > > > 
> > > > This cannot be the reason for it though, as we cannot run tasks while a
> page
> > > is
> > > > being swept?
> > > 
> > > Yeah, agreed.
> > > 
> > > I was thinking that one theoretical possibility is that some destructor
> > executes
> > > JS and a micro task is scheduled after the JS execution. However, that
> cannot
> > > happen because we are in ScriptForbiddenScope while sweeping objects.
> > > 
> > > Although it seems true that this CL fixes the crash (as far as I run
> > dom-modify
> > > 10 times), we need to consider a bit more to figure out what's going on
> here.
> > 
> > I don't mind switching as eager finalization is now preferred over
is*Swept(),
> > lgtm, but I don't see&understand the bug.
> 
> I see this ImageLoader task being run() with DOMTimer::fired() further up the
> stack. It is unsafe unless you're based on a tree having
> https://codereview.chromium.org/1160123004/ (or newer.)

So it's now safe to land the CL?

[sof, 2015-06-09 12:39:27]

On 2015/06/09 12:20:48, haraken wrote:
> On 2015/06/09 12:16:58, sof wrote:
> > On 2015/06/09 11:18:15, sof wrote:
> > > On 2015/06/09 11:13:30, haraken wrote:
> > > > On 2015/06/09 11:06:38, sof wrote:
> > > > > On 2015/06/09 11:03:47, haraken wrote:
> > > > > > As commented in https://codereview.chromium.org/1151163002/#msg13,
> > > > > > Heap::willObjectBeLazilySwept can return a wrong result in some edge
> > > cases.
> > > > I
> > > > > > think ImageLoader is hitting the cases. This CL removes
> > > > > > Heap::willObjectBeLazilySwept from ImageLoader and avoids that
> problem.
> > > > > > 
> > > > > > I confirmed that the crash in dom-modify is gone.
> > > > > > 
> > > > > > PTAL.
> > > > > 
> > > > > This cannot be the reason for it though, as we cannot run tasks while
a
> > page
> > > > is
> > > > > being swept?
> > > > 
> > > > Yeah, agreed.
> > > > 
> > > > I was thinking that one theoretical possibility is that some destructor
> > > executes
> > > > JS and a micro task is scheduled after the JS execution. However, that
> > cannot
> > > > happen because we are in ScriptForbiddenScope while sweeping objects.
> > > > 
> > > > Although it seems true that this CL fixes the crash (as far as I run
> > > dom-modify
> > > > 10 times), we need to consider a bit more to figure out what's going on
> > here.
> > > 
> > > I don't mind switching as eager finalization is now preferred over
> is*Swept(),
> > > lgtm, but I don't see&understand the bug.
> > 
> > I see this ImageLoader task being run() with DOMTimer::fired() further up
the
> > stack. It is unsafe unless you're based on a tree having
> > https://codereview.chromium.org/1160123004/ (or newer.)
> 
> So it's now safe to land the CL?

Yes, no problem there -- but the chain of calls that trigger this isn't clear.
But if it is a problem, it might not be limited to ImageResource, but Resource?

[haraken, 2015-06-09 12:55:14]

> Yes, no problem there -- but the chain of calls that trigger this isn't clear.
> But if it is a problem, it might not be limited to ImageResource, but
Resource?

Would the trace yutak posted be helpful?

#0  blink::Member<blink::LocalFrame>::operator blink::LocalFrame* (this=0x15f2)
at ../../third_party/WebKit/Source/platform/heap/Handle.h:694
#1  0x000055555a47aff5 in blink::Document::settings (this=0x1402) at
../../third_party/WebKit/Source/core/dom/Document.cpp:1479
#2  0x000055555ad93694 in blink::ImageLoader::getImageAnimationPolicy
(this=0x252314d79e88, policy=@0x7fffffffa074:
blink::ImageAnimationPolicyAllowed)
    at ../../third_party/WebKit/Source/core/loader/ImageLoader.cpp:628
#3  0x000055555ab46d0f in blink::ImageResource::updateImageAnimationPolicy
(this=0x205b9e01c520)
    at ../../third_party/WebKit/Source/core/fetch/ImageResource.cpp:436
#4  0x000055555ad92f92 in blink::ImageLoader::notifyFinished
(this=0x5af984328e8, resource=0x205b9e01c520)
    at ../../third_party/WebKit/Source/core/loader/ImageLoader.cpp:457
#5  0x000055555a6d62ac in blink::HTMLImageLoader::notifyFinished
(this=0x5af984328e8) at
../../third_party/WebKit/Source/core/html/HTMLImageLoader.cpp:82
#6  0x000055555ab5e9ae in blink::Resource::didAddClient (this=0x205b9e01c520,
c=0x5af984328e8) at ../../third_party/WebKit/Source/core/fetch/Resource.cpp:536
#7  0x000055555ab44f23 in blink::ImageResource::didAddClient
(this=0x205b9e01c520, c=0x5af984328e8)
    at ../../third_party/WebKit/Source/core/fetch/ImageResource.cpp:105
#8  0x000055555ab5e720 in blink::Resource::addClient (this=0x205b9e01c520,
client=0x5af984328e8) at
../../third_party/WebKit/Source/core/fetch/Resource.cpp:530
#9  0x000055555ad9266f in blink::ImageLoader::doUpdateFromElement
(this=0x5af984328e8, bypassBehavior=blink::ImageLoader::DoNotBypassMainWorldCSP,

    updateBehavior=blink::ImageLoader::UpdateIgnorePreviousError) at
../../third_party/WebKit/Source/core/loader/ImageLoader.cpp:360
#10 0x000055555ad92bef in blink::ImageLoader::updateFromElement
(this=0x5af984328e8,
updateBehavior=blink::ImageLoader::UpdateIgnorePreviousError)
    at ../../third_party/WebKit/Source/core/loader/ImageLoader.cpp:394
#11 0x000055555a6cfd3b in blink::HTMLImageElement::selectSourceURL
(this=0x31d584ce3bf8, behavior=blink::ImageLoader::UpdateIgnorePreviousError)
    at ../../third_party/WebKit/Source/core/html/HTMLImageElement.cpp:666
#12 0x000055555a6d0a2d in blink::HTMLImageElement::parseAttribute
(this=0x31d584ce3bf8, name="src", value="http://www.w3.org/Icons/w3c_home")
    at ../../third_party/WebKit/Source/core/html/HTMLImageElement.cpp:267
#13 0x000055555a4e7c97 in blink::Element::attributeChanged (this=0x31d584ce3bf8,
name="src", newValue="http://www.w3.org/Icons/w3c_home", 
    reason=blink::Element::ModifiedDirectly) at
../../third_party/WebKit/Source/core/dom/Element.cpp:1107
#14 0x000055555a4f40cd in blink::Element::attributeChangedFromParserOrByCloning
(this=0x31d584ce3bf8, name="src", newValue="http://www.w3.org/Icons/w3c_home", 
    reason=blink::Element::ModifiedDirectly) at
../../third_party/WebKit/Source/core/dom/Element.cpp:1161
#15 0x000055555a4e8d25 in blink::Element::parserSetAttributes
(this=0x31d584ce3bf8, attributeVector=WTF::Vector of length 4, capacity 4 =
{...})
    at ../../third_party/WebKit/Source/core/dom/Element.cpp:1297
#16 0x000055555a8e9113 in blink::setAttributes (element=0x31d584ce3bf8,
token=0x7fffffffb4d8, parserContentPolicy=blink::AllowScriptingContent)
    at
../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:66
#17 0x000055555a8ea3f2 in blink::HTMLConstructionSite::createHTMLElement
(this=0x5af98429c58, token=0x7fffffffb4d8)
    at
../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:757
#18 0x000055555a8ea82a in
blink::HTMLConstructionSite::insertSelfClosingHTMLElement (this=0x5af98429c58,
token=0x7fffffffb4d8)
    at
../../third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:633
#19 0x000055555a869011 in blink::HTMLTreeBuilder::processStartTagForInBody
(this=0x5af98429c40, token=0x7fffffffb4d8)
    at ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:791
#20 0x000055555a862fa2 in blink::HTMLTreeBuilder::processStartTag
(this=0x5af98429c40, token=0x7fffffffb4d8)
    at ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:1143
#21 0x000055555a86231a in blink::HTMLTreeBuilder::processToken
(this=0x5af98429c40, token=0x7fffffffb4d8)
    at ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:417
#22 0x000055555a8613bb in blink::HTMLTreeBuilder::constructTree
(this=0x5af98429c40, token=0x7fffffffb4d8)
    at ../../third_party/WebKit/Source/core/html/parser/HTMLTreeBuilder.cpp:377
#23 0x000055555a829c01 in
blink::HTMLDocumentParser::constructTreeFromCompactHTMLToken
(this=0x5af984298c8, compactToken=...)
    at
../../third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:710
#24 0x000055555a8297cd in
blink::HTMLDocumentParser::processParsedChunkFromBackgroundParser
(this=0x5af984298c8, popChunk=...)
    at
../../third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:492
#25 0x000055555a827ff8 in blink::HTMLDocumentParser::pumpPendingSpeculations
(this=0x5af984298c8)
    at
../../third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:563
#26 0x000055555a827b3c in blink::HTMLDocumentParser::resumeParsingAfterYield
(this=0x5af984298c8)
    at
../../third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:307
#27 0x000055555a83d968 in blink::HTMLParserScheduler::continueParsing
(this=0x2faeb0afdd90)
    at
../../third_party/WebKit/Source/core/html/parser/HTMLParserScheduler.cpp:165
#28 0x000055555a83e179 in WTF::FunctionWrapper<void
(blink::HTMLParserScheduler::*)()>::operator() (this=0x180e2ffe4568,
c=0x2faeb0afdd90)
    at ../../third_party/WebKit/Source/wtf/Functional.h:83
#29 0x000055555a83e100 in WTF::PartBoundFunctionImpl<1,
WTF::FunctionWrapper<void (blink::HTMLParserScheduler::*)()>, void
(blink::HTMLParserScheduler*)>::operator()() (this=0x180e2ffe4560) at
../../third_party/WebKit/Source/wtf/Functional.h:178
#30 0x00005555617465ea in blink::CancellableTaskFactory::CancellableTask::run
(this=0x180e2ecc06b0)
    at
../../third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.cpp:29
#31 0x000055555db04bee in scheduler::WebSchedulerImpl::runTask (task=...) at
../../components/scheduler/child/web_scheduler_impl.cc:46
#32 0x000055555db06aca in base::internal::RunnableAdapter<void
(*)(scoped_ptr<blink::WebThread::Task,
base::DefaultDeleter<blink::WebThread::Task> >)>::Run (
    this=0x7fffffffb888, args=...) at ../../base/bind_internal.h:157
#33 0x000055555db06a02 in base::internal::InvokeHelper<false, void,
base::internal::RunnableAdapter<void (*)(scoped_ptr<blink::WebThread::Task,
base::DefaultDeleter<blink::WebThread::Task> >)>,
base::internal::TypeList<scoped_ptr<blink::WebThread::Task,
base::DefaultDeleter<blink::WebThread::Task> > > >::MakeItSo (
    runnable=..., args=...) at ../../base/bind_internal.h:293
#34 0x000055555db069b3 in base::internal::Invoker<base::IndexSequence<0ul>,
base::internal::BindState<base::internal::RunnableAdapter<void
(*)(scoped_ptr<blink::WebThread::Task,
base::DefaultDeleter<blink::WebThread::Task> >)>, void
(scoped_ptr<blink::WebThread::Task, base::DefaultDeleter<blink::WebThread::Task>
>),
base::internal::TypeList<base::internal::PassedWrapper<scoped_ptr<blink::WebThread::Task,
base::DefaultDeleter<blink::WebThread::Task> > > > >,
base::internal::TypeList<base::internal::UnwrapTraits<base::internal::PassedWrapper<scoped_ptr<blink::WebThread::Task,
base::DefaultDeleter<blink::WebThread::Task> > > > >,
base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void
(*)(scoped_ptr<blink::WebThread::Task,
base::DefaultDeleter<blink::WebThread::Task> >)>,
base::internal::TypeList<scoped_ptr<blink::WebThread::Task,
base::DefaultDeleter<blink::WebThread::Task> > > >, void
()>::Run(base::internal::BindStateBase*) (base=0x180e2f864790) at
../../base/bind_internal.h:343
#35 0x000055555625359e in base::Callback<void ()>::Run() const
(this=0x7fffffffbb38) at ../../base/callback.h:396
#36 0x00005555575f7498 in base::debug::TaskAnnotator::RunTask
(this=0x180e2df44664, queue_function=0x555563d3f3d6 <.L.str.26>
"TaskQueueManager::PostTask", 
    run_function=0x555563d3f3f1 <.L.str.27> "TaskQueueManager::RunTask",
pending_task=...) at ../../base/debug/task_annotator.cc:62
#37 0x000055555dafb86d in scheduler::TaskQueueManager::ProcessTaskFromWorkQueue
(this=0x180e2df44620, queue_index=5, has_previous_task=false, 
    previous_task=0x7fffffffbdc0) at
../../components/scheduler/child/task_queue_manager.cc:674

[sof, 2015-06-09 13:07:47]

On 2015/06/09 12:39:27, sof wrote:
> On 2015/06/09 12:20:48, haraken wrote:
> > On 2015/06/09 12:16:58, sof wrote:
> > > On 2015/06/09 11:18:15, sof wrote:
> > > > On 2015/06/09 11:13:30, haraken wrote:
> > > > > On 2015/06/09 11:06:38, sof wrote:
> > > > > > On 2015/06/09 11:03:47, haraken wrote:
> > > > > > > As commented in https://codereview.chromium.org/1151163002/#msg13,
> > > > > > > Heap::willObjectBeLazilySwept can return a wrong result in some
edge
> > > > cases.
> > > > > I
> > > > > > > think ImageLoader is hitting the cases. This CL removes
> > > > > > > Heap::willObjectBeLazilySwept from ImageLoader and avoids that
> > problem.
> > > > > > > 
> > > > > > > I confirmed that the crash in dom-modify is gone.
> > > > > > > 
> > > > > > > PTAL.
> > > > > > 
> > > > > > This cannot be the reason for it though, as we cannot run tasks
while
> a
> > > page
> > > > > is
> > > > > > being swept?
> > > > > 
> > > > > Yeah, agreed.
> > > > > 
> > > > > I was thinking that one theoretical possibility is that some
destructor
> > > > executes
> > > > > JS and a micro task is scheduled after the JS execution. However, that
> > > cannot
> > > > > happen because we are in ScriptForbiddenScope while sweeping objects.
> > > > > 
> > > > > Although it seems true that this CL fixes the crash (as far as I run
> > > > dom-modify
> > > > > 10 times), we need to consider a bit more to figure out what's going
on
> > > here.
> > > > 
> > > > I don't mind switching as eager finalization is now preferred over
> > is*Swept(),
> > > > lgtm, but I don't see&understand the bug.
> > > 
> > > I see this ImageLoader task being run() with DOMTimer::fired() further up
> the
> > > stack. It is unsafe unless you're based on a tree having
> > > https://codereview.chromium.org/1160123004/ (or newer.)
> > 
> > So it's now safe to land the CL?
> 
> Yes, no problem there -- but the chain of calls that trigger this isn't clear.
> But if it is a problem, it might not be limited to ImageResource, but
Resource?

https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/0c5d38NpdSg/R72...
has the stack showing up the problem & I'm seeing another due to cloneNode()
triggering ImageLoader::notifyFinished().

(It is a bit odd that ImageLoader::m_image isn't traced.)

[haraken, 2015-06-09 13:15:09]

On 2015/06/09 13:07:47, sof wrote:
> On 2015/06/09 12:39:27, sof wrote:
> > On 2015/06/09 12:20:48, haraken wrote:
> > > On 2015/06/09 12:16:58, sof wrote:
> > > > On 2015/06/09 11:18:15, sof wrote:
> > > > > On 2015/06/09 11:13:30, haraken wrote:
> > > > > > On 2015/06/09 11:06:38, sof wrote:
> > > > > > > On 2015/06/09 11:03:47, haraken wrote:
> > > > > > > > As commented in
https://codereview.chromium.org/1151163002/#msg13,
> > > > > > > > Heap::willObjectBeLazilySwept can return a wrong result in some
> edge
> > > > > cases.
> > > > > > I
> > > > > > > > think ImageLoader is hitting the cases. This CL removes
> > > > > > > > Heap::willObjectBeLazilySwept from ImageLoader and avoids that
> > > problem.
> > > > > > > > 
> > > > > > > > I confirmed that the crash in dom-modify is gone.
> > > > > > > > 
> > > > > > > > PTAL.
> > > > > > > 
> > > > > > > This cannot be the reason for it though, as we cannot run tasks
> while
> > a
> > > > page
> > > > > > is
> > > > > > > being swept?
> > > > > > 
> > > > > > Yeah, agreed.
> > > > > > 
> > > > > > I was thinking that one theoretical possibility is that some
> destructor
> > > > > executes
> > > > > > JS and a micro task is scheduled after the JS execution. However,
that
> > > > cannot
> > > > > > happen because we are in ScriptForbiddenScope while sweeping
objects.
> > > > > > 
> > > > > > Although it seems true that this CL fixes the crash (as far as I run
> > > > > dom-modify
> > > > > > 10 times), we need to consider a bit more to figure out what's going
> on
> > > > here.
> > > > > 
> > > > > I don't mind switching as eager finalization is now preferred over
> > > is*Swept(),
> > > > > lgtm, but I don't see&understand the bug.
> > > > 
> > > > I see this ImageLoader task being run() with DOMTimer::fired() further
up
> > the
> > > > stack. It is unsafe unless you're based on a tree having
> > > > https://codereview.chromium.org/1160123004/ (or newer.)
> > > 
> > > So it's now safe to land the CL?
> > 
> > Yes, no problem there -- but the chain of calls that trigger this isn't
clear.
> > But if it is a problem, it might not be limited to ImageResource, but
> Resource?
> 
>
https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/0c5d38NpdSg/R72...
> has the stack showing up the problem & I'm seeing another due to cloneNode()
> triggering ImageLoader::notifyFinished().
> 
> (It is a bit odd that ImageLoader::m_image isn't traced.)

Yeah, it seems strange. Actually the Resource hierarchy is in a half-baked state
from the Oilpan perspective, and we have to complete the migration.

[sof, 2015-06-09 13:17:48]

On 2015/06/09 12:55:14, haraken wrote:
> > Yes, no problem there -- but the chain of calls that trigger this isn't
clear.
> > But if it is a problem, it might not be limited to ImageResource, but
> Resource?
> 
> Would the trace yutak posted be helpful?
> 
> #0  blink::Member<blink::LocalFrame>::operator blink::LocalFrame*
(this=0x15f2)
> at ../../third_party/WebKit/Source/platform/heap/Handle.h:694
...
>

Thanks much, yes - will look more closely.

[commit-bot: I haz the power, 2015-06-09 13:26:04]

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

[sof, 2015-06-09 14:23:40]

On 2015/06/09 13:15:09, haraken wrote:
> On 2015/06/09 13:07:47, sof wrote:
> >
...
> >
>
https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/0c5d38NpdSg/R72...
> > has the stack showing up the problem & I'm seeing another due to cloneNode()
> > triggering ImageLoader::notifyFinished().
> > 
> > (It is a bit odd that ImageLoader::m_image isn't traced.)
> 
> Yeah, it seems strange. Actually the Resource hierarchy is in a half-baked
state
> from the Oilpan perspective, and we have to complete the migration.

Yes.

I understand the problem now -- the loop over ImageResourceClients in
ImageResource::updateImageAnimationPolicy() might access a lazy-sweepable client
(ImageLoader). Which may refer to an m_element that has already been
finalized/swept; that explains the crash site.

So, the handling of ResourceClients that are on the heap needs to be considered
some.

[sof, 2015-06-10 09:23:40]

I suspected this would happen at some point, but eagerly finalizing ImageLoader
might access another eagerly finalized object, LocalDOMWindow in some cases.

Reproduces w/ ASan on PageSerializerTest.SVGImageDontCrash

[sof, 2015-06-10 10:35:47]

On 2015/06/10 09:23:40, sof wrote:
> I suspected this would happen at some point, but eagerly finalizing
ImageLoader
> might access another eagerly finalized object, LocalDOMWindow in some cases.
> 
> Reproduces w/ ASan on PageSerializerTest.SVGImageDontCrash

Stack shown here also,

https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...

[haraken, 2015-06-10 10:37:28]

On 2015/06/10 10:35:47, sof wrote:
> On 2015/06/10 09:23:40, sof wrote:
> > I suspected this would happen at some point, but eagerly finalizing
> ImageLoader
> > might access another eagerly finalized object, LocalDOMWindow in some cases.
> > 
> > Reproduces w/ ASan on PageSerializerTest.SVGImageDontCrash
> 
> Stack shown here also,
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...

Yeah, this is something I've been afraid of... The pre-finalizer would be a
solution but...

[sof, 2015-06-10 10:59:37]

On 2015/06/10 10:37:28, haraken wrote:
> On 2015/06/10 10:35:47, sof wrote:
> > On 2015/06/10 09:23:40, sof wrote:
> > > I suspected this would happen at some point, but eagerly finalizing
> > ImageLoader
> > > might access another eagerly finalized object, LocalDOMWindow in some
cases.
> > > 
> > > Reproduces w/ ASan on PageSerializerTest.SVGImageDontCrash
> > 
> > Stack shown here also,
> > 
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...
> 
> Yeah, this is something I've been afraid of... The pre-finalizer would be a
> solution but...

For commonly accessed objects (LocalDOMWindow), prefinalizers are perhaps the
preferable option. Use eager finalization for objects that are at the boundary
of the Oilpan heap, needing to unregister or let go of (external) references
before the mutator resumes after a GC.

[haraken, 2015-06-10 11:14:15]

On 2015/06/10 10:59:37, sof wrote:
> On 2015/06/10 10:37:28, haraken wrote:
> > On 2015/06/10 10:35:47, sof wrote:
> > > On 2015/06/10 09:23:40, sof wrote:
> > > > I suspected this would happen at some point, but eagerly finalizing
> > > ImageLoader
> > > > might access another eagerly finalized object, LocalDOMWindow in some
> cases.
> > > > 
> > > > Reproduces w/ ASan on PageSerializerTest.SVGImageDontCrash
> > > 
> > > Stack shown here also,
> > > 
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...
> > 
> > Yeah, this is something I've been afraid of... The pre-finalizer would be a
> > solution but...
> 
> For commonly accessed objects (LocalDOMWindow), prefinalizers are perhaps the
> preferable option. Use eager finalization for objects that are at the boundary
> of the Oilpan heap, needing to unregister or let go of (external) references
> before the mutator resumes after a GC.

If we have to use pre-finalizers in some cases, I'd prefer using pre-finalizers
everywhere to reduce the oilpan's programming tax (but I'm not yet convinced
that we should go back to the pre-finalizers).

The disadvantages of pre-finalizers are:

(a) We need to introduce a dispose() method.
(b) Pre-finalizers are not reentrant.

Regarding (b), is there any case we have to support reentrant pre-finalizers?
Can we just forbid the reentrancy?

[sof, 2015-06-10 11:33:09]

On 2015/06/10 11:14:15, haraken wrote:
> On 2015/06/10 10:59:37, sof wrote:
> > On 2015/06/10 10:37:28, haraken wrote:
> > > On 2015/06/10 10:35:47, sof wrote:
> > > > On 2015/06/10 09:23:40, sof wrote:
> > > > > I suspected this would happen at some point, but eagerly finalizing
> > > > ImageLoader
> > > > > might access another eagerly finalized object, LocalDOMWindow in some
> > cases.
> > > > > 
> > > > > Reproduces w/ ASan on PageSerializerTest.SVGImageDontCrash
> > > > 
> > > > Stack shown here also,
> > > > 
> > > >
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...
> > > 
> > > Yeah, this is something I've been afraid of... The pre-finalizer would be
a
> > > solution but...
> > 
> > For commonly accessed objects (LocalDOMWindow), prefinalizers are perhaps
the
> > preferable option. Use eager finalization for objects that are at the
boundary
> > of the Oilpan heap, needing to unregister or let go of (external) references
> > before the mutator resumes after a GC.
> 
> If we have to use pre-finalizers in some cases, I'd prefer using
pre-finalizers
> everywhere to reduce the oilpan's programming tax (but I'm not yet convinced
> that we should go back to the pre-finalizers).
> 
> The disadvantages of pre-finalizers are:
> 
> (a) We need to introduce a dispose() method.
> (b) Pre-finalizers are not reentrant.
> 
> Regarding (b), is there any case we have to support reentrant pre-finalizers?
> Can we just forbid the reentrancy?

s/re-entrant/nested/ ? Have a look at
https://codereview.chromium.org/1148383012/

You would have the exact same problem here if both ImageLoader and
LocalDOMWindow were "prefinalized".

I'm going to drop back to a prefinalizer for LocalDOMWindow; we can't have this
bug around.

[sof, 2015-06-10 11:38:10]

On 2015/06/10 11:33:09, sof wrote:
> On 2015/06/10 11:14:15, haraken wrote:
> > On 2015/06/10 10:59:37, sof wrote:
> > > On 2015/06/10 10:37:28, haraken wrote:
> > > > On 2015/06/10 10:35:47, sof wrote:
> > > > > On 2015/06/10 09:23:40, sof wrote:
> > > > > > I suspected this would happen at some point, but eagerly finalizing
> > > > > ImageLoader
> > > > > > might access another eagerly finalized object, LocalDOMWindow in
some
> > > cases.
> > > > > > 
> > > > > > Reproduces w/ ASan on PageSerializerTest.SVGImageDontCrash
> > > > > 
> > > > > Stack shown here also,
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...
> > > > 
> > > > Yeah, this is something I've been afraid of... The pre-finalizer would
be
> a
> > > > solution but...
> > > 
> > > For commonly accessed objects (LocalDOMWindow), prefinalizers are perhaps
> the
> > > preferable option. Use eager finalization for objects that are at the
> boundary
> > > of the Oilpan heap, needing to unregister or let go of (external)
references
> > > before the mutator resumes after a GC.
> > 
> > If we have to use pre-finalizers in some cases, I'd prefer using
> pre-finalizers
> > everywhere to reduce the oilpan's programming tax (but I'm not yet convinced
> > that we should go back to the pre-finalizers).
> > 
> > The disadvantages of pre-finalizers are:
> > 
> > (a) We need to introduce a dispose() method.
> > (b) Pre-finalizers are not reentrant.
> > 
> > Regarding (b), is there any case we have to support reentrant
pre-finalizers?
> > Can we just forbid the reentrancy?
> 
> s/re-entrant/nested/ ? Have a look at
> https://codereview.chromium.org/1148383012/
> 
> You would have the exact same problem here if both ImageLoader and
> LocalDOMWindow were "prefinalized".
> 

Actually not for that pair of destructors, but you still have to be careful to
make prefinalizers work across any execution ordering.

[haraken, 2015-06-10 13:30:10]

(Let me think about the pre-finalizer issues a bit more.)

BTW, this bug was use-after-free errors caused by lazy sweeping. That is scary.

To detect those use-after-frees errors more easily, I implemented a mechanism to
defer reusing free lists for one GC cycle
(https://codereview.chromium.org/1176003002/). This mechanism has been working
only on ASan builds in a half-baked state, but the CL enables the mechanism in
all ENABLE(ASSERT) builds. Hope it helps.

[sof, 2015-06-10 14:03:35]

On 2015/06/10 13:30:10, haraken wrote:
> (Let me think about the pre-finalizer issues a bit more.)
> 
> BTW, this bug was use-after-free errors caused by lazy sweeping. That is
scary.
> 
> To detect those use-after-frees errors more easily, I implemented a mechanism
to
> defer reusing free lists for one GC cycle
> (https://codereview.chromium.org/1176003002/). This mechanism has been working
> only on ASan builds in a half-baked state, but the CL enables the mechanism in
> all ENABLE(ASSERT) builds. Hope it helps.

The ASan poisoning for the eager heap is as strict as it can be, but fully agree
with the above.