[Oilpan] [Reland] Migrate classes under core/fetch to Oilpan heap

Reland of crrev.com/1108753002
Migrate classes under core/fetch to Oilpan heap


BUG=479467, 497658
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198461

[peria, 2015-06-04 09:32:43]

Patchset #1 (id:1) has been deleted

[peria, 2015-06-04 09:32:57]

Patchset #1 (id:20001) has been deleted

[peria, 2015-06-04 09:33:06]

Patchset #1 (id:40001) has been deleted

[peria, 2015-06-04 09:33:21]

Patchset #1 (id:60001) has been deleted

[peria, 2015-06-04 09:33:33]

Patchset #1 (id:80001) has been deleted

[peria, 2015-06-04 09:33:42]

Patchset #1 (id:100001) has been deleted

[peria, 2015-06-04 09:33:52]

Patchset #1 (id:120001) has been deleted

[peria, 2015-06-04 09:33:59]

Patchset #1 (id:140001) has been deleted

[peria, 2015-06-04 09:39:15]

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

[peria, 2015-06-04 09:39:16]

Just make it public to share current status.

I have to know why "if(foo.isEmpty()) foo.clear()"
in ResourceFetcher.cpp solves a memory leak. :(

[sof, 2015-06-08 14:45:39]

On 2015/06/04 09:39:16, peria wrote:
> Just make it public to share current status.
> 
> I have to know why "if(foo.isEmpty()) foo.clear()"
> in ResourceFetcher.cpp solves a memory leak. :(

The backing storage might not be shrunk/rehashed right away, I suppose, but that
should not bring about a considerable leak. Have you made further inroads into
understanding why?

[peria, 2015-06-09 03:09:57]

On 2015/06/08 14:45:39, sof wrote:
> On 2015/06/04 09:39:16, peria wrote:
> > Just make it public to share current status.
> > 
> > I have to know why "if(foo.isEmpty()) foo.clear()"
> > in ResourceFetcher.cpp solves a memory leak. :(
> 
> The backing storage might not be shrunk/rehashed right away, I suppose, but
that
> should not bring about a considerable leak. Have you made further inroads into
> understanding why?

Yes, I tried to trace how m_documentResources is handled in ToT with gdb, but I
failed.
As far as I watched, it seems not released obviously.

[haraken, 2015-06-09 04:35:32]

On 2015/06/08 14:45:39, sof wrote:
> On 2015/06/04 09:39:16, peria wrote:
> > Just make it public to share current status.
> > 
> > I have to know why "if(foo.isEmpty()) foo.clear()"
> > in ResourceFetcher.cpp solves a memory leak. :(
> 
> The backing storage might not be shrunk/rehashed right away, I suppose, but
that
> should not bring about a considerable leak. Have you made further inroads into
> understanding why?

Is it possible that isEmpty returns true while there are elements that need to
be shrunk/rehashed? (I'm asking this without reading the code.)

[peria, 2015-06-09 04:41:21]

On 2015/06/09 04:35:32, haraken wrote:
> On 2015/06/08 14:45:39, sof wrote:
> > On 2015/06/04 09:39:16, peria wrote:
> > > Just make it public to share current status.
> > > 
> > > I have to know why "if(foo.isEmpty()) foo.clear()"
> > > in ResourceFetcher.cpp solves a memory leak. :(
> > 
> > The backing storage might not be shrunk/rehashed right away, I suppose, but
> that
> > should not bring about a considerable leak. Have you made further inroads
into
> > understanding why?
> 
> Is it possible that isEmpty returns true while there are elements that need to
> be shrunk/rehashed? (I'm asking this without reading the code.)

No, isEmpty returns true if and only if no keys are stored.
And in such a case, backing storage can be allocated.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[peria, 2015-06-09 04:53:44]

On 2015/06/09 04:41:21, peria wrote:
> On 2015/06/09 04:35:32, haraken wrote:
> > On 2015/06/08 14:45:39, sof wrote:
> > > On 2015/06/04 09:39:16, peria wrote:
> > > > Just make it public to share current status.
> > > > 
> > > > I have to know why "if(foo.isEmpty()) foo.clear()"
> > > > in ResourceFetcher.cpp solves a memory leak. :(
> > > 
> > > The backing storage might not be shrunk/rehashed right away, I suppose,
but
> > that
> > > should not bring about a considerable leak. Have you made further inroads
> into
> > > understanding why?
> > 
> > Is it possible that isEmpty returns true while there are elements that need
to
> > be shrunk/rehashed? (I'm asking this without reading the code.)
> 
> No, isEmpty returns true if and only if no keys are stored.
> And in such a case, backing storage can be allocated.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Oh, I'm sorry, I was wrong.
It can happen.

1) Assume HashMap<K,V> has one entry.
2) Remove the entry, then the hash map has no keys (isEmpty() == true), but the
backing storage keeps a value alive.
3) Add one entry to the hash map.
4) The backing storage will be rehashed.
5) In rehashTo(), all values in the backing entry will be moved into a new
backing storage.

Let me check if this situation really happens.

[sof, 2015-06-09 05:18:41]

On 2015/06/09 04:53:44, peria wrote:
> On 2015/06/09 04:41:21, peria wrote:
> > On 2015/06/09 04:35:32, haraken wrote:
> > > On 2015/06/08 14:45:39, sof wrote:
> > > > On 2015/06/04 09:39:16, peria wrote:
> > > > > Just make it public to share current status.
> > > > > 
> > > > > I have to know why "if(foo.isEmpty()) foo.clear()"
> > > > > in ResourceFetcher.cpp solves a memory leak. :(
> > > > 
> > > > The backing storage might not be shrunk/rehashed right away, I suppose,
> but
> > > that
> > > > should not bring about a considerable leak. Have you made further
inroads
> > into
> > > > understanding why?
> > > 
> > > Is it possible that isEmpty returns true while there are elements that
need
> to
> > > be shrunk/rehashed? (I'm asking this without reading the code.)
> > 
> > No, isEmpty returns true if and only if no keys are stored.
> > And in such a case, backing storage can be allocated.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Oh, I'm sorry, I was wrong.
> It can happen.
> 
> 1) Assume HashMap<K,V> has one entry.
> 2) Remove the entry, then the hash map has no keys (isEmpty() == true), but
the
> backing storage keeps a value alive.
> 3) Add one entry to the hash map.
> 4) The backing storage will be rehashed.
> 5) In rehashTo(), all values in the backing entry will be moved into a new
> backing storage.
> 
> Let me check if this situation really happens.

Deleting the entry should zero it out with an empty/deleted value in the hash
table, but perhaps something unexpected happens for ResourcePtr<> slots?

[peria, 2015-06-09 05:22:20]

On 2015/06/09 05:18:41, sof wrote:
> On 2015/06/09 04:53:44, peria wrote:
> > Oh, I'm sorry, I was wrong.
> > It can happen.
> > 
> > 1) Assume HashMap<K,V> has one entry.
> > 2) Remove the entry, then the hash map has no keys (isEmpty() == true), but
> the
> > backing storage keeps a value alive.
> > 3) Add one entry to the hash map.
> > 4) The backing storage will be rehashed.
> > 5) In rehashTo(), all values in the backing entry will be moved into a new
> > backing storage.
> > 
> > Let me check if this situation really happens.
> 
> Deleting the entry should zero it out with an empty/deleted value in the hash
> table,
Yes, every entry is checked if it is deleted.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
so (5) in my guess was wrong.

> but perhaps something unexpected happens for ResourcePtr<> slots?
I'll check it.

[peria, 2015-06-09 06:27:52]

after today's rebase, this CL seems pass ASAN tests without the strange clear().
Let me try it on bots.

[peria, 2015-06-11 06:57:48]

Now leaking objects are OwnPtrs for closures stored in
ResourceFetcher -> Timer -> CancellableTaskFactory.
Those closures seem not released in trunk code, though...

[peria, 2015-06-17 09:20:16]

Patchset #6 (id:250001) has been deleted

[haraken, 2015-06-18 05:31:29]

I cannot reproduce this leak in my local build. What's a GYP_DEFINE I need to
use? Or you cannot reproduce it on ToT either?

[haraken, 2015-06-18 05:31:55]

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

[haraken, 2015-06-18 05:31:56]

Re-adding Sigbjorn and haraken

[peria, 2015-06-18 05:39:08]

On 2015/06/18 05:31:29, haraken wrote:
> I cannot reproduce this leak in my local build. What's a GYP_DEFINE I need to
> use? Or you cannot reproduce it on ToT either?

  GYP_DEFIENS="asan=1 lsan=1"
is enough to build a chromium which reproduce this leak.
To see the leak report, you have to set ASAN_OPTIONS.
Assuming your $PWD is /path/to/chromium/src/,
  ASAN_OPTIONS="detect_leaks=1 symbolize=1
external_symbolizer_path=${PWD}/third_party/llvm-build/Release+Asserts/bin/llvm-symbolizer"
out/Debug/content_browsertests
--gtest_filter="RenderViewImplTest.DidFailProvisionalLoadWithErrorForCancellation"
works.

[haraken, 2015-06-18 05:55:34]

On 2015/06/18 05:39:08, peria wrote:
> On 2015/06/18 05:31:29, haraken wrote:
> > I cannot reproduce this leak in my local build. What's a GYP_DEFINE I need
to
> > use? Or you cannot reproduce it on ToT either?
> 
>   GYP_DEFIENS="asan=1 lsan=1"
> is enough to build a chromium which reproduce this leak.
> To see the leak report, you have to set ASAN_OPTIONS.
> Assuming your $PWD is /path/to/chromium/src/,
>   ASAN_OPTIONS="detect_leaks=1 symbolize=1
>
external_symbolizer_path=${PWD}/third_party/llvm-build/Release+Asserts/bin/llvm-symbolizer"
> out/Debug/content_browsertests
>
--gtest_filter="RenderViewImplTest.DidFailProvisionalLoadWithErrorForCancellation"
> works.

Thanks, now I can reproduce it.

[haraken, 2015-06-18 06:04:08]

On 2015/06/18 05:55:34, haraken wrote:
> On 2015/06/18 05:39:08, peria wrote:
> > On 2015/06/18 05:31:29, haraken wrote:
> > > I cannot reproduce this leak in my local build. What's a GYP_DEFINE I need
> to
> > > use? Or you cannot reproduce it on ToT either?
> > 
> >   GYP_DEFIENS="asan=1 lsan=1"
> > is enough to build a chromium which reproduce this leak.
> > To see the leak report, you have to set ASAN_OPTIONS.
> > Assuming your $PWD is /path/to/chromium/src/,
> >   ASAN_OPTIONS="detect_leaks=1 symbolize=1
> >
>
external_symbolizer_path=${PWD}/third_party/llvm-build/Release+Asserts/bin/llvm-symbolizer"
> > out/Debug/content_browsertests
> >
>
--gtest_filter="RenderViewImplTest.DidFailProvisionalLoadWithErrorForCancellation"
> > works.
> 
> Thanks, now I can reproduce it.

Hmm, if we call m_fetcher.clear() in Document::detach, the ResourceFetcher leak
is gone. However, something is still leaking...

==26772==ERROR: LeakSanitizer: detected memory leaks

Indirect leak of 519 byte(s) in 50 object(s) allocated from:
    #0 0x4fb34b in __interceptor_malloc
(/usr/local/google/home/haraken/chromium/src/out/Debug/content_browsertests+0x4fb34b)
    #1 0x7f41d105c839 in __strdup /build/buildd/eglibc-2.19/string/strdup.c:42


What I noticed is that if we apply this CL to ToT, the order in which the
Documents and ResourceFetchers get destructed changes.

(ToT)
ResourceFetcher 0x61400000f840
Document 0x61f000008c80
ResourceFetcher 0x61400000ee40
~Document 0x61f000008c80
~ResourceFetcher 0x61400000f840
Document 0x61f000007e80
// ResourceFetcher 0x61400000ee40 is leaking.

(ToT + this CL without m_fetcher.clear())
ResourceFetcher 0x2767d6901830
Document 0x61f000008c80
ResourceFetcher 0x2767d69019b8
~Document 0x61f000008c80
Document 0x61f000007e80   <-------- Note: This happens first before
~ResourceFetcher. This might be just because ~ResourceFetcher is delayed to the
next GC though.
~ResourceFetcher 0x2767d6901830
// ResourceFetcher 0x2767d69019b8 is leaking.

(ToT + this CL including m_fetcher.clear())
ResourceFetcher 0x2767d6901830
Document 0x61f000008c80
ResourceFetcher 0x2767d69019b8
~Document 0x61f000008c80
Document 0x61f000007e80
~ResourceFetcher 0x2767d6901830
~ResourceFetcher 0x2767d69019b8

[peria, 2015-06-18 06:08:52]

On 2015/06/18 06:04:08, haraken wrote:
> 
> Hmm, if we call m_fetcher.clear() in Document::detach, the ResourceFetcher
leak
> is gone. However, something is still leaking...
> 
> ==26772==ERROR: LeakSanitizer: detected memory leaks
> 
> Indirect leak of 519 byte(s) in 50 object(s) allocated from:
>     #0 0x4fb34b in __interceptor_malloc
>
(/usr/local/google/home/haraken/chromium/src/out/Debug/content_browsertests+0x4fb34b)
>     #1 0x7f41d105c839 in __strdup /build/buildd/eglibc-2.19/string/strdup.c:42
> 
> 

This leak is suppressed on trybot and we can ignore it.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/t...

To supreess it, set LSAN_OPTIONS as follows.
  ASAN_OPTIONS="...."
LSAN_OPTIONS="suppressions=${PWD}/third_party/skia/tools/lsan.supp"
./out/Debug/content_browsertests ....

[sof, 2015-06-18 06:16:41]

On 2015/06/18 06:04:08, haraken wrote:
> On 2015/06/18 05:55:34, haraken wrote:
> > On 2015/06/18 05:39:08, peria wrote:
> > > On 2015/06/18 05:31:29, haraken wrote:
> > > > I cannot reproduce this leak in my local build. What's a GYP_DEFINE I
need
> > to
> > > > use? Or you cannot reproduce it on ToT either?
> > > 
> > >   GYP_DEFIENS="asan=1 lsan=1"
> > > is enough to build a chromium which reproduce this leak.
> > > To see the leak report, you have to set ASAN_OPTIONS.
> > > Assuming your $PWD is /path/to/chromium/src/,
> > >   ASAN_OPTIONS="detect_leaks=1 symbolize=1
> > >
> >
>
external_symbolizer_path=${PWD}/third_party/llvm-build/Release+Asserts/bin/llvm-symbolizer"
> > > out/Debug/content_browsertests
> > >
> >
>
--gtest_filter="RenderViewImplTest.DidFailProvisionalLoadWithErrorForCancellation"
> > > works.
> > 
> > Thanks, now I can reproduce it.
> 
> Hmm, if we call m_fetcher.clear() in Document::detach, the ResourceFetcher
leak
> is gone. However, something is still leaking...
> 
> ==26772==ERROR: LeakSanitizer: detected memory leaks
> 
> Indirect leak of 519 byte(s) in 50 object(s) allocated from:
>     #0 0x4fb34b in __interceptor_malloc
>
(/usr/local/google/home/haraken/chromium/src/out/Debug/content_browsertests+0x4fb34b)
>     #1 0x7f41d105c839 in __strdup /build/buildd/eglibc-2.19/string/strdup.c:42
> 
> 
> What I noticed is that if we apply this CL to ToT, the order in which the
> Documents and ResourceFetchers get destructed changes.
> 
> (ToT)
> ResourceFetcher 0x61400000f840
> Document 0x61f000008c80
> ResourceFetcher 0x61400000ee40
> ~Document 0x61f000008c80
> ~ResourceFetcher 0x61400000f840
> Document 0x61f000007e80
> // ResourceFetcher 0x61400000ee40 is leaking.
> 
> (ToT + this CL without m_fetcher.clear())
> ResourceFetcher 0x2767d6901830
> Document 0x61f000008c80
> ResourceFetcher 0x2767d69019b8
> ~Document 0x61f000008c80
> Document 0x61f000007e80   <-------- Note: This happens first before
> ~ResourceFetcher. This might be just because ~ResourceFetcher is delayed to
the
> next GC though.
> ~ResourceFetcher 0x2767d6901830
> // ResourceFetcher 0x2767d69019b8 is leaking.
> 
> (ToT + this CL including m_fetcher.clear())
> ResourceFetcher 0x2767d6901830
> Document 0x61f000008c80
> ResourceFetcher 0x2767d69019b8
> ~Document 0x61f000008c80
> Document 0x61f000007e80
> ~ResourceFetcher 0x2767d6901830
> ~ResourceFetcher 0x2767d69019b8

Thanks, I think those dtor sequences help indicate that a Document is released
very late by these tests, causing the resources it hangs onto to be released
even later. And too late if it involves a heap object with an OwnPtr<>
allocation.

Clearing m_fetcher in detach() isn't a solution; working out what those two
Documents are and if it is possible to release them earlier might be worth
looking into?

[haraken, 2015-06-18 06:45:49]

> Thanks, I think those dtor sequences help indicate that a Document is released
> very late by these tests, causing the resources it hangs onto to be released
> even later. And too late if it involves a heap object with an OwnPtr<>
> allocation.
> 
> Clearing m_fetcher in detach() isn't a solution; working out what those two
> Documents are and if it is possible to release them earlier might be worth
> looking into?

More data.

(ToT)
ResourceFetcher 0x61400000f840
Document 0x61f000008c80
ResourceFetcher 0x61400000ee40
Document::detach 0x61f000008c80  // (a)
~Document 0x61f000008c80
~ResourceFetcher 0x61400000f840
Document 0x61f000007e80
Document::detach 0x61f000007e80  // (b)

(ToT + this CL without m_fetcher.clear())
ResourceFetcher 0x1fd560f41830
Document 0x61f000008c80
ResourceFetcher 0x1fd560f419b8
Document::detach 0x61f000008c80  // (c)
~Document 0x61f000008c80
Document 0x61f000007e80
~ResourceFetcher 0x1fd560f41830
Document::detach 0x61f000007e80  // (d)

Stack trace of (a) and (c):
#0  blink::Document::detach (this=0x61f000008c80, context=...)
    at ../../third_party/WebKit/Source/core/dom/Document.cpp:2111
#1  0x000000000dba9c93 in blink::FrameLoader::prepareForCommit
(this=0x61700000f638)
    at ../../third_party/WebKit/Source/core/loader/FrameLoader.cpp:1038
#2  0x000000000dbaa153 in blink::FrameLoader::commitProvisionalLoad
(this=0x61700000f638)
    at ../../third_party/WebKit/Source/core/loader/FrameLoader.cpp:1055
#3  0x000000000db6d7d2 in blink::DocumentLoader::commitIfReady
(this=0x61c00000f080)
    at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:220
#4  0x000000000db72367 in blink::DocumentLoader::dataReceived
(this=0x61c00000f080, resource=0x61a00001e680, 
    data=0x602000025e90 "test data", length=9) at
../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:552
#5  0x000000000d680026 in blink::RawResource::appendData (this=0x61a00001e680,
data=0x602000025e90 "test data", length=9)
    at ../../third_party/WebKit/Source/core/fetch/RawResource.cpp:119
#6  0x000000000d6dd233 in blink::ResourceLoader::didReceiveData
(this=0x61a00001e080, data=0x602000025e90 "test data", 
    length=9, encodedDataLength=0) at
../../third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:438
#7  0x0000000010d7b036 in content::WebURLLoaderImpl::Context::OnReceivedData
(this=0x60c00001df80, 
    data=(content::RequestPeer::ReceivedData *) 0x604000024390) at 


Stack trace of (b) and (d):
#0  blink::Document::detach (this=0x61f000007e80, context=...)
    at ../../third_party/WebKit/Source/core/dom/Document.cpp:2111
#1  0x000000000d7edc64 in blink::LocalFrame::detach (this=0x61700000f580,
type=blink::FrameDetachType::Remove)
    at ../../third_party/WebKit/Source/core/frame/LocalFrame.cpp:290
#2  0x000000000dc4f1c6 in blink::Page::willBeDestroyed (this=0x61300000c280)
    at ../../third_party/WebKit/Source/core/page/Page.cpp:560
#3  0x000000000adc008b in blink::WebViewImpl::close (this=0x61700000f900)
    at ../../third_party/WebKit/Source/web/WebViewImpl.cpp:1679
#4  0x000000000170a4d8 in content::RenderWidget::Close (this=0x61d000014080)
    at ../../content/renderer/render_widget.cc:1586
#5  0x000000000164eb18 in content::RenderViewImpl::Close (this=0x61d000014080)
    at ../../content/renderer/render_view_impl.cc:3046


So the timing where Document::detach() is called are the same between ToT and
ToT + this CL.

It seems that the first Document is a Document for about:blank (which is
replaced once the main Document is loaded). The second Document is the main
Document of the test.

> Clearing m_fetcher in detach() isn't a solution; working out what those two
> Documents are and if it is possible to release them earlier might be worth
> looking into?

Just help me understand but why do you think calling m_fetcher.clear() in
Document::detach() is not a solution?

[peria, 2015-06-18 06:53:20]

On 2015/06/18 06:45:49, haraken wrote:
> It seems that the first Document is a Document for about:blank (which is
> replaced once the main Document is loaded). The second Document is the main
> Document of the test.

It seems not correct.
Destruction of the first Document and construction of the second Document
happen in TearDown(), so the second one is not used.

(ToT)
> in TearDown()
~Document(0x61f000008c80)
~ResourceFetcher(0x61400000f840)
Document(0x61f000007e80)

(ToT + this CL)
> in TearDown()
~Document(0x61f000008c80)
Document(0x61f000007e80)
~ResourceFetcher(0x7f07e13a1830)

[haraken, 2015-06-18 07:12:55]

The first Document is allocated in SetUp():
#0  blink::Document::Document (this=0x61f000008c80, initializer=...,
documentClasses=1 '\001')
    at ../../third_party/WebKit/Source/core/dom/Document.cpp:480
#1  0x000000000cbda438 in blink::HTMLDocument::HTMLDocument
(this=0x61f000008c80, initializer=..., 
    extendedDocumentClasses=0 '\000') at
../../third_party/WebKit/Source/core/html/HTMLDocument.cpp:72
#2  0x000000000c2530d8 in blink::HTMLDocument::create (initializer=...)
    at ../../third_party/WebKit/Source/core/html/HTMLDocument.h:39
#3  0x000000000c252085 in blink::DOMImplementation::createDocument (type=...,
init=..., inViewSourceMode=false)
    at ../../third_party/WebKit/Source/core/dom/DOMImplementation.cpp:226
#4  0x000000000d7c9785 in blink::LocalDOMWindow::createDocument (mimeType=...,
init=..., forceXHTML=false)
    at ../../third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:330
#5  0x000000000d7c9b5f in blink::LocalDOMWindow::installNewDocument
(this=0x61500000a800, mimeType=..., init=..., 
    forceXHTML=false) at
../../third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:344
#6  0x000000000db7e69e in blink::DocumentLoader::createWriterFor
(ownerDocument=0x0, init=..., mimeType=..., 
    encoding=..., dispatch=false, parsingPolicy=blink::AllowAsynchronousParsing)
    at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:781
#7  0x000000000db7e13a in blink::DocumentLoader::ensureWriter
(this=0x61c00000f880, mimeType=..., overridingURL=...)
    at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:508
#8  0x000000000db7afe9 in blink::DocumentLoader::commitData
(this=0x61c00000f880, bytes=0x0, length=0)
    at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:522
#9  0x000000000db7a85d in blink::DocumentLoader::finishedLoading
(this=0x61c00000f880, finishTime=198783.42762900001)
    at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:268
#10 0x000000000db7fc98 in blink::DocumentLoader::maybeLoadEmpty
(this=0x61c00000f880)
    at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:688
#11 0x000000000db801f9 in blink::DocumentLoader::startLoadingMainResource
(this=0x61c00000f880)
    at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:701
#12 0x000000000dbac27b in blink::FrameLoader::init (this=0x61700000f638)
    at ../../third_party/WebKit/Source/core/loader/FrameLoader.cpp:193
#13 0x000000000ad0ba0c in blink::LocalFrame::init (this=0x61700000f580)
    at ../../third_party/WebKit/Source/core/frame/LocalFrame.h:234
#14 0x000000000ad019c7 in blink::WebLocalFrameImpl::initializeCoreFrame
(this=0x611000050f40, host=0x606000029ba0, owner=
    0x0, name=..., fallbackName=...) at
../../third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1713
#15 0x000000000ada5f2e in blink::WebViewImpl::setMainFrame (this=0x61700000f900,
frame=0x611000050f40)
    at ../../third_party/WebKit/Source/web/WebViewImpl.cpp:349
#16 0x000000000160a5b9 in content::RenderViewImpl::Initialize (this=<optimized
out>, params=..., 
    compositor_deps=<optimized out>, was_created_by_renderer=<optimized out>)
    at ../../content/renderer/render_view_impl.cc:719
#17 0x000000000161a541 in content::RenderViewImpl::Create (params=...,
compositor_deps=0x61500000b200, 
    was_created_by_renderer=false) at
../../content/renderer/render_view_impl.cc:1151
#18 0x00000000028abd04 in content::RenderViewTest::SetUp (this=<optimized out>)
    at ../../content/public/test/render_view_test.cc:260
#19 0x0000000000c1dea6 in content::RenderViewImplTest::SetUp
(this=0x61500000d500)
    at ../../content/renderer/render_view_browsertest.cc:154
#20 0x00000000035a4ae4 in
testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>
()
    at ../../testing/gtest/src/gtest.cc:2364

[haraken, 2015-06-18 07:13:05]

The second Document is created in TearDown():
#0  blink::Document::Document (this=0x61f000007e80, initializer=...,
documentClasses=1 '\001')
    at ../../third_party/WebKit/Source/core/dom/Document.cpp:480
#1  0x000000000cbda438 in blink::HTMLDocument::HTMLDocument
(this=0x61f000007e80, initializer=..., 
    extendedDocumentClasses=0 '\000') at
../../third_party/WebKit/Source/core/html/HTMLDocument.cpp:72
#2  0x000000000cd1a7b7 in blink::HTMLViewSourceDocument::HTMLViewSourceDocument
(this=0x61f000007e80, initializer=..., 
    mimeType=...) at
../../third_party/WebKit/Source/core/html/HTMLViewSourceDocument.cpp:54
#3  0x000000000c2535cf in blink::HTMLViewSourceDocument::create
(initializer=..., mimeType=...)
    at ../../third_party/WebKit/Source/core/html/HTMLViewSourceDocument.h:45
#4  0x000000000c25202c in blink::DOMImplementation::createDocument (type=...,
init=..., inViewSourceMode=true)
    at ../../third_party/WebKit/Source/core/dom/DOMImplementation.cpp:222
#5  0x000000000d7c9785 in blink::LocalDOMWindow::createDocument (mimeType=...,
init=..., forceXHTML=false)
    at ../../third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:330
#6  0x000000000d7c9b5f in blink::LocalDOMWindow::installNewDocument
(this=0x61500000a800, mimeType=..., init=..., 
    forceXHTML=false) at
../../third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:344
#7  0x000000000db7e69e in blink::DocumentLoader::createWriterFor
(ownerDocument=0x0, init=..., mimeType=..., 
    encoding=..., dispatch=false, parsingPolicy=blink::AllowAsynchronousParsing)
    at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:781
#8  0x000000000db7e13a in blink::DocumentLoader::ensureWriter
(this=0x61c00000f080, mimeType=..., overridingURL=...)
    at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:508
#9  0x000000000db7afe9 in blink::DocumentLoader::commitData
(this=0x61c00000f080, bytes=0x602000025950 "test data", 
    length=9) at
../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:522
#10 0x000000000db7edb1 in blink::DocumentLoader::dataReceived
(this=0x61c00000f080, resource=0x61a00001e680, 
    data=0x602000025950 "test data", length=9) at
../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:555
#11 0x000000000d688a26 in blink::RawResource::appendData (this=0x61a00001e680,
data=0x602000025950 "test data", length=9)
    at ../../third_party/WebKit/Source/core/fetch/RawResource.cpp:119
#12 0x000000000d6e7cc6 in blink::ResourceLoader::didReceiveData
(this=0x7fffc2a61bf8, data=0x602000025950 "test data", 
    length=9, encodedDataLength=0) at
../../third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:425
#13 0x0000000010d87ad6 in content::WebURLLoaderImpl::Context::OnReceivedData
(this=0x60c00001df80, 
    data=(content::RequestPeer::ReceivedData *) 0x604000024310) at
../../content/child/web_url_loader_impl.cc:693
#14 0x0000000010d7beff in content::WebURLLoaderImpl::Context::HandleDataURL
(this=0x60c00001df80)
    at ../../content/child/web_url_loader_impl.cc:811
#15 0x0000000010d8d16b in base::internal::RunnableAdapter<void
(content::WebURLLoaderImpl::Context::*)()>::Run (
    this=0x7fffea5d07e0, object=0x60c00001df80) at
../../base/bind_internal.h:176
#16 0x0000000010d8cfee in base::internal::InvokeHelper<false, void,
base::internal::RunnableAdapter<void (content::WebURLLoaderImpl::Context::*)()>,
base::internal::TypeList<content::WebURLLoaderImpl::Context* const&> >::MakeItSo
(
    runnable=..., args=<optimized out>) at ../../base/bind_internal.h:293
#17 0x0000000010d8cdf5 in base::internal::Invoker<base::IndexSequence<0ul>,
base::internal::BindState<base::internal::RunnableAdapter<void
(content::WebURLLoaderImpl::Context::*)()>, void
(content::WebURLLoaderImpl::Context*),
base::internal::TypeList<content::WebURLLoaderImpl::Context*> >,
base::internal::TypeList<base::internal::UnwrapTraits<content::WebURLLoaderImpl::Context*>
>, base::internal::InvokeHelper<false, void,
base::internal::RunnableAdapter<void (content::WebURLLoaderImpl::Context::*)()>,
base::internal::TypeList<content::WebURLLoaderImpl::Context* const&> >, void
()>::Run(base::internal::BindStateBase*) (base=0x604000025350) at
../../base/bind_internal.h:343
#18 0x00000000005c6454 in base::Callback<void ()>::Run() const
(this=0x7fffeaa3c1d8) at ../../base/callback.h:396
#19 0x00000000043d592f in base::debug::TaskAnnotator::RunTask
(this=0x611000053c64, 
    queue_function=0x1b3a23a0 <.str.26> "TaskQueueManager::PostTask", 
    run_function=0x1b3a23e0 <.str.27> "TaskQueueManager::RunTask",
pending_task=...)
    at ../../base/debug/task_annotator.cc:62
#20 0x0000000010f8813e in scheduler::TaskQueueManager::ProcessTaskFromWorkQueue
(this=0x611000053c40, queue_index=0, 
    has_previous_task=false, previous_task=0x7fffeaa38710) at
../../components/scheduler/child/task_queue_manager.cc:674
#21 0x0000000010f83c8c in scheduler::TaskQueueManager::DoWork
(this=0x611000053c40, posted_from_main_thread=true)
    at ../../components/scheduler/child/task_queue_manager.cc:627
#22 0x0000000010fab4c7 in base::internal::RunnableAdapter<void
(scheduler::TaskQueueManager::*)(bool)>::Run (
    this=0x7fffea5cc8a0, object=0x611000053c40, args=@0x606000000e98: true) at
../../base/bind_internal.h:176
eueManager::*)(bool)>,
base::internal::TypeList<base::WeakPtr<scheduler::TaskQueueManager> const&, bool
const&> >::MakeItSo (runnable=..., weak_ptr=..., args=<optimized out>) at
../../base/bind_internal.h:303
#24 0x0000000010fab04c in base::internal::Invoker<base::IndexSequence<0ul, 1ul>,
base::internal::BindState<base::internal::RunnableAdapter<void
(scheduler::TaskQueueManager::*)(bool)>, void (scheduler::TaskQueueManager*,
bool), base::internal::TypeList<base::WeakPtr<scheduler::TaskQueueManager>,
bool> >,
base::internal::TypeList<base::internal::UnwrapTraits<base::WeakPtr<scheduler::TaskQueueManager>
>, base::internal::UnwrapTraits<bool> >, base::internal::InvokeHelper<true,
void, base::internal::RunnableAdapter<void
(scheduler::TaskQueueManager::*)(bool)>,
base::internal::TypeList<base::WeakPtr<scheduler::TaskQueueManager> const&, bool
const&> >, void ()>::Run(base::internal::BindStateBase*) (base=0x606000000e60)
    at ../../base/bind_internal.h:343
#25 0x00000000005c6454 in base::Callback<void ()>::Run() const
(this=0x7fffea830d38) at ../../base/callback.h:396
#26 0x00000000043d592f in base::debug::TaskAnnotator::RunTask
(this=0x61500000d640, 
    queue_function=0x1949e480 <.str.14> "MessageLoop::PostTask", 
    run_function=0x1949e4c0 <.str.15> "MessageLoop::RunTask", pending_task=...)
at ../../base/debug/task_annotator.cc:62
#27 0x000000000400805c in base::MessageLoop::RunTask (this=0x61500000d510,
pending_task=...)
    at ../../base/message_loop/message_loop.cc:458
#28 0x000000000400845c in base::MessageLoop::DeferOrRunPendingTask
(this=0x61500000d510, pending_task=...)
    at ../../base/message_loop/message_loop.cc:468
#29 0x00000000040097ce in base::MessageLoop::DoWork (this=0x61500000d510) at
../../base/message_loop/message_loop.cc:580
#30 0x000000000403c638 in base::MessagePumpDefault::Run (this=0x60800001ba20,
delegate=0x61500000d510)
    at ../../base/message_loop/message_pump_default.cc:34
#31 0x00000000040071e0 in base::MessageLoop::RunHandler (this=0x61500000d510)
    at ../../base/message_loop/message_loop.cc:424
#32 0x00000000040c994c in base::RunLoop::Run (this=0x7fffea90bfa0) at
../../base/run_loop.cc:55
#33 0x00000000040056ab in base::MessageLoop::Run (this=0x61500000d510) at
../../base/message_loop/message_loop.cc:286
#34 0x00000000028a85e5 in content::RenderViewTest::ProcessPendingMessages
(this=0x61500000d500)
    at ../../content/public/test/render_view_test.cc:140
#35 0x00000000028ac04a in content::RenderViewTest::TearDown
(this=0x61500000d500)

[sof, 2015-06-18 07:17:05]

Good stuff, getting closer :)

Have you tried adding a call to blink::WebHeap::collectGarbageForTesting() after
the 2nd round of GCs in TearDown() (after ProcessPendingMessages())?

[haraken, 2015-06-18 07:36:35]

On 2015/06/18 07:17:05, sof wrote:
> Good stuff, getting closer :)
> 
> Have you tried adding a call to blink::WebHeap::collectGarbageForTesting()
after
> the 2nd round of GCs in TearDown() (after ProcessPendingMessages())?

We already have it. Also I tried to add
blink::WebHeap::collectGarbageForTesting() after
render_thread_->SendCloseMessage(), but the situation is the same...

[haraken, 2015-06-18 07:43:16]

Here is my theory:

(a) The ResourceFetcher associated with the second Document is leaking in both
ToT and ToT + this CL.
(b) That is just because the second Document is leaking in both ToT and ToT +
this CL.
(c) Document::detach for the second Document is called in both ToT and ToT +
this CL.
(d) LSan detects the ResourceFetcher leak in ToT + this CL, but doesn't detect
the ResourceFetcher leak in ToT for some reason.

The core problem is (b) -- the second Document is leaking in both ToT and ToT +
this CL. However, this wouldn't be related to Oilpan.

So it might make sense to go with the following approach:

(1) Call m_fetcher.clear in Document::detach (I guess it's OK). This will solve
the problem (a), (c) and (d). (b) remains though.
(2) If calling m_fetcher.clear in Document::detach is not allowed for some
reason, give up the leak fix and just suppress it.

What do you think? (Though I think we should spend a bit more time on
investigating the reason the second Document is leaking.)

[sof, 2015-06-18 07:46:04]

On 2015/06/18 07:43:16, haraken wrote:
> Here is my theory:
> 
> (a) The ResourceFetcher associated with the second Document is leaking in both
> ToT and ToT + this CL.
> (b) That is just because the second Document is leaking in both ToT and ToT +
> this CL.
> (c) Document::detach for the second Document is called in both ToT and ToT +
> this CL.
> (d) LSan detects the ResourceFetcher leak in ToT + this CL, but doesn't detect
> the ResourceFetcher leak in ToT for some reason.
> 
> The core problem is (b) -- the second Document is leaking in both ToT and ToT
+
> this CL. However, this wouldn't be related to Oilpan.
> 
> So it might make sense to go with the following approach:
> 
> (1) Call m_fetcher.clear in Document::detach (I guess it's OK). This will
solve
> the problem (a), (c) and (d). (b) remains though.
> (2) If calling m_fetcher.clear in Document::detach is not allowed for some
> reason, give up the leak fix and just suppress it.
> 
> What do you think? (Though I think we should spend a bit more time on
> investigating the reason the second Document is leaking.)

(1) is failing on some crazy extensions test -- 


http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

so retaining m_fetcher until ~Document runs appears needed.

[haraken, 2015-06-18 07:50:45]

On 2015/06/18 07:46:04, sof wrote:
> On 2015/06/18 07:43:16, haraken wrote:
> > Here is my theory:
> > 
> > (a) The ResourceFetcher associated with the second Document is leaking in
both
> > ToT and ToT + this CL.
> > (b) That is just because the second Document is leaking in both ToT and ToT
+
> > this CL.
> > (c) Document::detach for the second Document is called in both ToT and ToT +
> > this CL.
> > (d) LSan detects the ResourceFetcher leak in ToT + this CL, but doesn't
detect
> > the ResourceFetcher leak in ToT for some reason.
> > 
> > The core problem is (b) -- the second Document is leaking in both ToT and
ToT
> +
> > this CL. However, this wouldn't be related to Oilpan.
> > 
> > So it might make sense to go with the following approach:
> > 
> > (1) Call m_fetcher.clear in Document::detach (I guess it's OK). This will
> solve
> > the problem (a), (c) and (d). (b) remains though.
> > (2) If calling m_fetcher.clear in Document::detach is not allowed for some
> > reason, give up the leak fix and just suppress it.
> > 
> > What do you think? (Though I think we should spend a bit more time on
> > investigating the reason the second Document is leaking.)
> 
> (1) is failing on some crazy extensions test -- 
> 
> 
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> 
> so retaining m_fetcher until ~Document runs appears needed.

At a first glance at the stack trace, that looks like another bug.

@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@==1931==ERROR: AddressSanitizer: SEGV
on unknown address 0x000000000040 (pc 0x000009dd6228 bp 0x7fffb42fec90 sp
0x7fffb42fec70 T0)@@@
@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #0 0x9dd6227 in operator!
third_party/WebKit/Source/wtf/OwnPtr.h:70:42@@@
@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #1 0x9dd6227 in
blink::ResourceFetcher::clearPreloads()
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:749@@@
@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #2 0x8fe6300 in
blink::Document::finishedParsing()
third_party/WebKit/Source/core/dom/Document.cpp:4603:5@@@
@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #3 0x9741ba0 in
blink::HTMLDocumentParser::end()
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:859:5@@@
@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #4 0x9730417 in
blink::HTMLDocumentParser::prepareToStopParsing()
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:272:5@@@
@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #5 0x973897b in
blink::HTMLDocumentParser::processParsedChunkFromBackgroundParser(WTF::PassOwnPtr\u003Cblink::HTMLDocumentParser::ParsedChunk>)
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:485:17@@@
@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #6 0x97334f6 in
blink::HTMLDocumentParser::pumpPendingSpeculations()
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:563:36@@@
@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #7 0x9742f58 in
blink::HTMLDocumentParser::resumeParsingAfterScriptExecution()
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:999:9@@@
@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #8 0x97436b2 in
blink::HTMLDocumentParser::notifyScriptLoaded(blink::Resource*)
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:1035:9@@@
@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #9 0x9dc4647 in
blink::Resource::checkNotify()
third_party/WebKit/Source/core/fetch/Resource.cpp:248:9@@@
@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #10 0x9dc5fcb in
blink::Resource::finish()
third_party/WebKit/Source/core/fetch/Resource.cpp:307:5@@@
@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #11 0x9df7535 in
blink::ResourceLoader::didFinishLoading(blink::WebURLLoader*, double, long)
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:443:5@@@
@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #12 0xbf9fd31 in
content::WebURLLoaderImpl::Context::OnCompletedRequest(int, bool, bool,
std::__1::basic_string\u003Cchar, std::__1::char_traits\u003Cchar>,
std::__1::allocator\u003Cchar> > const&, base::TimeTicks const&, long)
content/child/web_url_loader_impl.cc:742:7@@@

This indicates that HTMLParser is running on a Document that has been already
detached. I don't think this is valid. Parser should not run on an
already-detached Document.

[peria, 2015-06-18 09:02:11]

Patchset #9 (id:330001) has been deleted

[haraken, 2015-06-18 16:32:30]

On 2015/06/18 07:50:45, haraken wrote:
> On 2015/06/18 07:46:04, sof wrote:
> > On 2015/06/18 07:43:16, haraken wrote:
> > > Here is my theory:
> > > 
> > > (a) The ResourceFetcher associated with the second Document is leaking in
> both
> > > ToT and ToT + this CL.
> > > (b) That is just because the second Document is leaking in both ToT and
ToT
> +
> > > this CL.
> > > (c) Document::detach for the second Document is called in both ToT and ToT
+
> > > this CL.
> > > (d) LSan detects the ResourceFetcher leak in ToT + this CL, but doesn't
> detect
> > > the ResourceFetcher leak in ToT for some reason.
> > > 
> > > The core problem is (b) -- the second Document is leaking in both ToT and
> ToT
> > +
> > > this CL. However, this wouldn't be related to Oilpan.
> > > 
> > > So it might make sense to go with the following approach:
> > > 
> > > (1) Call m_fetcher.clear in Document::detach (I guess it's OK). This will
> > solve
> > > the problem (a), (c) and (d). (b) remains though.
> > > (2) If calling m_fetcher.clear in Document::detach is not allowed for some
> > > reason, give up the leak fix and just suppress it.
> > > 
> > > What do you think? (Though I think we should spend a bit more time on
> > > investigating the reason the second Document is leaking.)
> > 
> > (1) is failing on some crazy extensions test -- 
> > 
> > 
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> > 
> > so retaining m_fetcher until ~Document runs appears needed.
> 
> At a first glance at the stack trace, that looks like another bug.
> 
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@==1931==ERROR:
AddressSanitizer: SEGV
> on unknown address 0x000000000040 (pc 0x000009dd6228 bp 0x7fffb42fec90 sp
> 0x7fffb42fec70 T0)@@@
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #0 0x9dd6227 in operator!
> third_party/WebKit/Source/wtf/OwnPtr.h:70:42@@@
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #1 0x9dd6227 in
> blink::ResourceFetcher::clearPreloads()
> third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:749@@@
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #2 0x8fe6300 in
> blink::Document::finishedParsing()
> third_party/WebKit/Source/core/dom/Document.cpp:4603:5@@@
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #3 0x9741ba0 in
> blink::HTMLDocumentParser::end()
> third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:859:5@@@
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #4 0x9730417 in
> blink::HTMLDocumentParser::prepareToStopParsing()
> third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:272:5@@@
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #5 0x973897b in
>
blink::HTMLDocumentParser::processParsedChunkFromBackgroundParser(WTF::PassOwnPtr\u003Cblink::HTMLDocumentParser::ParsedChunk>)
> third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:485:17@@@
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #6 0x97334f6 in
> blink::HTMLDocumentParser::pumpPendingSpeculations()
> third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:563:36@@@
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #7 0x9742f58 in
> blink::HTMLDocumentParser::resumeParsingAfterScriptExecution()
> third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:999:9@@@
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #8 0x97436b2 in
> blink::HTMLDocumentParser::notifyScriptLoaded(blink::Resource*)
> third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:1035:9@@@
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #9 0x9dc4647 in
> blink::Resource::checkNotify()
> third_party/WebKit/Source/core/fetch/Resource.cpp:248:9@@@
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #10 0x9dc5fcb in
> blink::Resource::finish()
> third_party/WebKit/Source/core/fetch/Resource.cpp:307:5@@@
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #11 0x9df7535 in
> blink::ResourceLoader::didFinishLoading(blink::WebURLLoader*, double, long)
> third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:443:5@@@
> mailto:@@@STEP_LOG_LINE@CrazyExtensionTest.Crazy@    #12 0xbf9fd31 in
> content::WebURLLoaderImpl::Context::OnCompletedRequest(int, bool, bool,
> std::__1::basic_string\u003Cchar, std::__1::char_traits\u003Cchar>,
> std::__1::allocator\u003Cchar> > const&, base::TimeTicks const&, long)
> content/child/web_url_loader_impl.cc:742:7@@@
> 
> This indicates that HTMLParser is running on a Document that has been already
> detached. I don't think this is valid. Parser should not run on an
> already-detached Document.

peria-san: Would you add m_fetcher.clear to Document::detach and run full layout
tests? We might want to understand a list of problems associated with
m_fetcher.clear. If those problems are fixable ones, my proposal would be to fix
them and m_fetcher.clear to Document::detach.

[peria, 2015-06-18 16:50:53]

On 2015/06/18 16:32:30, haraken wrote:
> 
> peria-san: Would you add m_fetcher.clear to Document::detach and run full
layout
> tests? We might want to understand a list of problems associated with
> m_fetcher.clear. If those problems are fixable ones, my proposal would be to
fix
> them and m_fetcher.clear to Document::detach.

I think PS#8 matches your requirement to put m_fetcher.clear() in
Document::detach().
I triggered a few trybots.  If you want more bots, please add them.

[peria, 2015-06-19 04:32:11]

Patchset #9 (id:350001) has been deleted

[peria, 2015-06-19 05:27:29]

Patchset #9 (id:370001) has been deleted

[sof, 2015-06-22 07:50:45]

On 2015/06/18 16:50:53, peria wrote:
> On 2015/06/18 16:32:30, haraken wrote:
> > 
> > peria-san: Would you add m_fetcher.clear to Document::detach and run full
> layout
> > tests? We might want to understand a list of problems associated with
> > m_fetcher.clear. If those problems are fixable ones, my proposal would be to
> fix
> > them and m_fetcher.clear to Document::detach.
> 
> I think PS#8 matches your requirement to put m_fetcher.clear() in
> Document::detach().
> I triggered a few trybots.  If you want more bots, please add them.

That looked rather red; what's the current status?

https://codereview.chromium.org/1194003004/ will bring some rebase trouble, but
otherwise no impact on these leaks. However, it would be safe&worth calling
clear() on m_fetcher in DocumentLoader::detachFromFrame().

[peria, 2015-06-22 08:45:38]

On 2015/06/22 07:50:45, sof wrote:
> That looked rather red; what's the current status?
No positive progress.
I found a path in which a ResourceFetcher is kept
  Page->Frame(LocalFrame)->LocalDOMWindow->Document->ResourceFetcher
and instances of Page, Frame, and LocalDOMWindow are destructed.

And I tried to clear ResourceFetcher from dtors of those classes,
but it led other crashes in other tests.

As haraken@ said the point (b), in his comment, is the root cause.
And we have to know who holds references to a Document instance,
to solve this issue completely.
I also think this is independent from Oilpan, and we or other blink
staffs should fix it.
So I would like to vote (2; suppress this leak) in haraken's suggestion.


> https://codereview.chromium.org/1194003004/ will bring some rebase trouble,
but
> otherwise no impact on these leaks. However, it would be safe&worth calling
> clear() on m_fetcher in DocumentLoader::detachFromFrame().

Thank you for tha patch and the suggestion, but unfortunately, it didn't solve
this issue.
Document and DocumentLoader have Persistent<RF> independently, and we know
Document instance is also alive.  So we also have to clear the pointer that
Document has.

[haraken, 2015-06-22 09:31:27]

On 2015/06/22 08:45:38, peria wrote:
> On 2015/06/22 07:50:45, sof wrote:
> > That looked rather red; what's the current status?
> No positive progress.
> I found a path in which a ResourceFetcher is kept
>   Page->Frame(LocalFrame)->LocalDOMWindow->Document->ResourceFetcher
> and instances of Page, Frame, and LocalDOMWindow are destructed.
> 
> And I tried to clear ResourceFetcher from dtors of those classes,
> but it led other crashes in other tests.
> 
> As haraken@ said the point (b), in his comment, is the root cause.
> And we have to know who holds references to a Document instance,
> to solve this issue completely.
> I also think this is independent from Oilpan, and we or other blink
> staffs should fix it.
> So I would like to vote (2; suppress this leak) in haraken's suggestion.
> 
> 
> > https://codereview.chromium.org/1194003004/ will bring some rebase trouble,
> but
> > otherwise no impact on these leaks. However, it would be safe&worth calling
> > clear() on m_fetcher in DocumentLoader::detachFromFrame().
> 
> Thank you for tha patch and the suggestion, but unfortunately, it didn't solve
> this issue.
> Document and DocumentLoader have Persistent<RF> independently, and we know
> Document instance is also alive.  So we also have to clear the pointer that
> Document has.

What happens if you call m_fetcher.clear() in both Document::detach() and
DocumentLoader::detachFromFrame()?

[peria, 2015-06-22 09:48:15]

On 2015/06/22 09:31:27, haraken wrote:
> On 2015/06/22 08:45:38, peria wrote:
> > On 2015/06/22 07:50:45, sof wrote:
> > > That looked rather red; what's the current status?
> > No positive progress.
> > I found a path in which a ResourceFetcher is kept
> >   Page->Frame(LocalFrame)->LocalDOMWindow->Document->ResourceFetcher
> > and instances of Page, Frame, and LocalDOMWindow are destructed.
> > 
> > And I tried to clear ResourceFetcher from dtors of those classes,
> > but it led other crashes in other tests.
> > 
> > As haraken@ said the point (b), in his comment, is the root cause.
> > And we have to know who holds references to a Document instance,
> > to solve this issue completely.
> > I also think this is independent from Oilpan, and we or other blink
> > staffs should fix it.
> > So I would like to vote (2; suppress this leak) in haraken's suggestion.
> > 
> > 
> > > https://codereview.chromium.org/1194003004/ will bring some rebase
trouble,
> > but
> > > otherwise no impact on these leaks. However, it would be safe&worth
calling
> > > clear() on m_fetcher in DocumentLoader::detachFromFrame().
> > 
> > Thank you for tha patch and the suggestion, but unfortunately, it didn't
solve
> > this issue.
> > Document and DocumentLoader have Persistent<RF> independently, and we know
> > Document instance is also alive.  So we also have to clear the pointer that
> > Document has.
> 
> What happens if you call m_fetcher.clear() in both Document::detach() and
> DocumentLoader::detachFromFrame()?

It passes at least RenderViewImplTest.* in content_browsertests.
I upload the merged patch in https://codereview.chromium.org/1198963003/ and run
bots.

[peria, 2015-06-24 09:22:18]

On 2015/06/22 09:31:27, haraken wrote:
> What happens if you call m_fetcher.clear() in both Document::detach() and
> DocumentLoader::detachFromFrame()?

Thank you haraken and Sigbjorn.
Calling m_fetcher.clear() in Document::detach() and
DocumentLoader()::detachFromFrame()
resolves so many leaks.
But unfortunately, extension_unittest.ScriptContextSetTest.Lifecycle still fails
with the leak.
I tried to call both methods, there still be a leak.
I wonder there can be other references to Persistent<ResourceFetcher>...

[peria, 2015-06-24 09:48:33]

On 2015/06/24 09:22:18, peria wrote:
> But unfortunately, extension_unittest.ScriptContextSetTest.Lifecycle still
fails
> with the leak.
> I tried to call both methods, there still be a leak.
> I wonder there can be other references to Persistent<ResourceFetcher>...

Hmm, I cannot get why this leak happens.
I found
1) Document and DocumentLoader are the only class which has a reference to RF.
2) One instance for each class are created in this test.
and if I intentionally call Doc::detach() and DL::detachFF() at the end of the
test,
3) those methods are called actually.  It means m_fetcher.clear() is called in
both methods.
4) still the leak happens. :(

[haraken, 2015-06-24 11:48:08]

On 2015/06/24 09:48:33, peria wrote:
> On 2015/06/24 09:22:18, peria wrote:
> > But unfortunately, extension_unittest.ScriptContextSetTest.Lifecycle still
> fails
> > with the leak.
> > I tried to call both methods, there still be a leak.
> > I wonder there can be other references to Persistent<ResourceFetcher>...
> 
> Hmm, I cannot get why this leak happens.
> I found
> 1) Document and DocumentLoader are the only class which has a reference to RF.
> 2) One instance for each class are created in this test.
> and if I intentionally call Doc::detach() and DL::detachFF() at the end of the
> test,
> 3) those methods are called actually.  It means m_fetcher.clear() is called in
> both methods.
> 4) still the leak happens. :(

This is because Document::detach is not called for the document created in
ScriptContextSetTest.Lifecycle. Looking at the test, you can notice that the
main frame are not closed before the test shuts down. That is the cause of the
leak.

I uploaded a fix here: https://codereview.chromium.org/1210513002/

[peria, 2015-06-25 03:15:19]

On 2015/06/24 11:48:08, haraken wrote:
> On 2015/06/24 09:48:33, peria wrote:
> > On 2015/06/24 09:22:18, peria wrote:
> > > But unfortunately, extension_unittest.ScriptContextSetTest.Lifecycle still
> > fails
> > > with the leak.
> > > I tried to call both methods, there still be a leak.
> > > I wonder there can be other references to Persistent<ResourceFetcher>...
> > 
> > Hmm, I cannot get why this leak happens.
> > I found
> > 1) Document and DocumentLoader are the only class which has a reference to
RF.
> > 2) One instance for each class are created in this test.
> > and if I intentionally call Doc::detach() and DL::detachFF() at the end of
the
> > test,
> > 3) those methods are called actually.  It means m_fetcher.clear() is called
in
> > both methods.
> > 4) still the leak happens. :(
> 
> This is because Document::detach is not called for the document created in
> ScriptContextSetTest.Lifecycle. Looking at the test, you can notice that the
> main frame are not closed before the test shuts down. That is the cause of the
> leak.
> 
> I uploaded a fix here: https://codereview.chromium.org/1210513002/

Uh, I failed with only frame->close(), but your CL solves the leak.
Thank you!

[sof, 2015-06-26 07:21:45]

https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
File Source/core/loader/DocumentLoader.cpp (right):

https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
Source/core/loader/DocumentLoader.cpp:647: if (m_fetcher)
Trying to understand; could you explain why the m_fetcher accesses are null
checked now?

[peria, 2015-06-26 07:31:43]

PS15 is just a working around to see some tests need more investigation.

https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
File Source/core/loader/DocumentLoader.cpp (right):

https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
Source/core/loader/DocumentLoader.cpp:647: if (m_fetcher)
On 2015/06/26 07:21:45, sof wrote:
> Trying to understand; could you explain why the m_fetcher accesses are null
> checked now?

No theoretical backgroud. :(
I'm just looking if there can be other reasons to fail tests.

Because we introduced m_fetcher.clear() in detach(), some tests failed with
accessing nullptr'ed m_fetcher.

[peria, 2015-06-29 03:12:58]

Patchset #18 (id:560001) has been deleted

[sof, 2015-06-29 13:29:29]

https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
File Source/core/loader/DocumentLoader.cpp (right):

https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
Source/core/loader/DocumentLoader.cpp:647: if (m_fetcher)
On 2015/06/26 07:31:43, peria wrote:
> On 2015/06/26 07:21:45, sof wrote:
> > Trying to understand; could you explain why the m_fetcher accesses are null
> > checked now?
> 
> No theoretical backgroud. :(
> I'm just looking if there can be other reasons to fail tests.
> 
> Because we introduced m_fetcher.clear() in detach(), some tests failed with
> accessing nullptr'ed m_fetcher.

I can't see much going wrong when I revert those changes locally. I think we do
need the one in Document::finishedParsing(), but that might be it?

fast/files/workers/worker-read-blob-async.html is failing though; but that's
also failing with the last trybot runs here.

https://chromiumcodereview.appspot.com/1124153003/diff/580001/Source/core/fet...
File Source/core/fetch/FetchContext.h (right):

https://chromiumcodereview.appspot.com/1124153003/diff/580001/Source/core/fet...
Source/core/fetch/FetchContext.h:61: static FetchContext* create()
Should we try to get rid of this? It's not on trunk.

https://chromiumcodereview.appspot.com/1124153003/diff/580001/Source/core/fet...
File Source/core/fetch/ImageResourceTest.cpp (right):

https://chromiumcodereview.appspot.com/1124153003/diff/580001/Source/core/fet...
Source/core/fetch/ImageResourceTest.cpp:146: ResourceFetcher* fetcher =
ResourceFetcher::create(FetchContext::create());
Why are we passing FetchContext::create() instead of nullptr? There's another in
ResourceFetcherTest.cpp.

These were switched over to use the latter in r194827.

https://chromiumcodereview.appspot.com/1124153003/diff/580001/Source/core/fet...
File Source/core/fetch/RawResource.cpp (right):

https://chromiumcodereview.appspot.com/1124153003/diff/580001/Source/core/fet...
Source/core/fetch/RawResource.cpp:61: if (!fetcher)
Why is this needed here now?

https://chromiumcodereview.appspot.com/1124153003/diff/580001/Source/core/fet...
File Source/core/fetch/ResourceLoaderHost.h (right):

https://chromiumcodereview.appspot.com/1124153003/diff/580001/Source/core/fet...
Source/core/fetch/ResourceLoaderHost.h:2: * Copyright (C) 2013 Google Inc. All
rights reserved.
The ResourceLoaderHost was removed in r195503, so we can remove it from here
also.

[sof, 2015-06-29 15:00:47]

https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
File Source/core/loader/DocumentLoader.cpp (right):

https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
Source/core/loader/DocumentLoader.cpp:647: if (m_fetcher)
On 2015/06/29 13:29:29, sof wrote:
> On 2015/06/26 07:31:43, peria wrote:
> > On 2015/06/26 07:21:45, sof wrote:
> > > Trying to understand; could you explain why the m_fetcher accesses are
null
> > > checked now?
> > 
> > No theoretical backgroud. :(
> > I'm just looking if there can be other reasons to fail tests.
> > 
> > Because we introduced m_fetcher.clear() in detach(), some tests failed with
> > accessing nullptr'ed m_fetcher.
> 
> I can't see much going wrong when I revert those changes locally. I think we
do
> need the one in Document::finishedParsing(), but that might be it?
> 
> fast/files/workers/worker-read-blob-async.html is failing though; but that's
> also failing with the last trybot runs here.

Having looked at that test failure some, I'm wondering if the extra clearing of
ResourceFetchers added here needs to be reconsidered. i.e., a detached Document
ends up being used by a Worker to initiate a load -- finding no ResourceFetcher
when doing so.

[peria, 2015-06-30 07:14:39]

https://codereview.chromium.org/1124153003/diff/520001/Source/core/loader/Doc...
File Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/1124153003/diff/520001/Source/core/loader/Doc...
Source/core/loader/DocumentLoader.cpp:647: if (m_fetcher)
On 2015/06/29 13:29:29, sof wrote:
> On 2015/06/26 07:31:43, peria wrote:
> > On 2015/06/26 07:21:45, sof wrote:
> > > Trying to understand; could you explain why the m_fetcher accesses are
null
> > > checked now?
> > 
> > No theoretical backgroud. :(
> > I'm just looking if there can be other reasons to fail tests.
> > 
> > Because we introduced m_fetcher.clear() in detach(), some tests failed with
> > accessing nullptr'ed m_fetcher.
> 
> I can't see much going wrong when I revert those changes locally. I think we
do
> need the one in Document::finishedParsing(), but that might be it?
> 
> fast/files/workers/worker-read-blob-async.html is failing though; but that's
> also failing with the last trybot runs here.

Yes, we need this check in Document::finishedParsing().  It fixes a failure of
CrazyExtensionTest.Crazy.
I'm not sure if it is needed in other area, though.

https://codereview.chromium.org/1124153003/diff/580001/Source/core/fetch/Fetc...
File Source/core/fetch/FetchContext.h (right):

https://codereview.chromium.org/1124153003/diff/580001/Source/core/fetch/Fetc...
Source/core/fetch/FetchContext.h:61: static FetchContext* create()
On 2015/06/29 13:29:29, sof wrote:
> Should we try to get rid of this? It's not on trunk.

Done.

https://codereview.chromium.org/1124153003/diff/580001/Source/core/fetch/Imag...
File Source/core/fetch/ImageResourceTest.cpp (right):

https://codereview.chromium.org/1124153003/diff/580001/Source/core/fetch/Imag...
Source/core/fetch/ImageResourceTest.cpp:146: ResourceFetcher* fetcher =
ResourceFetcher::create(FetchContext::create());
On 2015/06/29 13:29:29, sof wrote:
> Why are we passing FetchContext::create() instead of nullptr? There's another
in
> ResourceFetcherTest.cpp.
> 
> These were switched over to use the latter in r194827.

Thank you for pointing it out.
I was not aware of the CL.

https://codereview.chromium.org/1124153003/diff/580001/Source/core/fetch/RawR...
File Source/core/fetch/RawResource.cpp (right):

https://codereview.chromium.org/1124153003/diff/580001/Source/core/fetch/RawR...
Source/core/fetch/RawResource.cpp:61: if (!fetcher)
On 2015/06/29 13:29:29, sof wrote:
> Why is this needed here now?

it seems not needed here.
removed.

https://codereview.chromium.org/1124153003/diff/580001/Source/core/fetch/Reso...
File Source/core/fetch/ResourceLoaderHost.h (right):

https://codereview.chromium.org/1124153003/diff/580001/Source/core/fetch/Reso...
Source/core/fetch/ResourceLoaderHost.h:2: * Copyright (C) 2013 Google Inc. All
rights reserved.
On 2015/06/29 13:29:29, sof wrote:
> The ResourceLoaderHost was removed in r195503, so we can remove it from here
> also.

Done.

[sof, 2015-06-30 07:39:40]

On 2015/06/29 15:00:47, sof wrote:
>
https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
> File Source/core/loader/DocumentLoader.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
> Source/core/loader/DocumentLoader.cpp:647: if (m_fetcher)
> On 2015/06/29 13:29:29, sof wrote:
> > On 2015/06/26 07:31:43, peria wrote:
> > > On 2015/06/26 07:21:45, sof wrote:
> > > > Trying to understand; could you explain why the m_fetcher accesses are
> null
> > > > checked now?
> > > 
> > > No theoretical backgroud. :(
> > > I'm just looking if there can be other reasons to fail tests.
> > > 
> > > Because we introduced m_fetcher.clear() in detach(), some tests failed
with
> > > accessing nullptr'ed m_fetcher.
> > 
> > I can't see much going wrong when I revert those changes locally. I think we
> do
> > need the one in Document::finishedParsing(), but that might be it?
> > 
> > fast/files/workers/worker-read-blob-async.html is failing though; but that's
> > also failing with the last trybot runs here.
> 
> Having looked at that test failure some, I'm wondering if the extra clearing
of
> ResourceFetchers added here needs to be reconsidered. i.e., a detached
Document
> ends up being used by a Worker to initiate a load -- finding no
ResourceFetcher
> when doing so.

Do you think the eager removal/clearing of the ResourceFetchers is stable&viable
overall?

We need to make forward progress.

[peria, 2015-06-30 08:08:27]

On 2015/06/30 07:39:40, sof wrote:
> On 2015/06/29 15:00:47, sof wrote:
> >
>
https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
> > File Source/core/loader/DocumentLoader.cpp (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
> > Source/core/loader/DocumentLoader.cpp:647: if (m_fetcher)
> > On 2015/06/29 13:29:29, sof wrote:
> > > On 2015/06/26 07:31:43, peria wrote:
> > > > On 2015/06/26 07:21:45, sof wrote:
> > > > > Trying to understand; could you explain why the m_fetcher accesses are
> > null
> > > > > checked now?
> > > > 
> > > > No theoretical backgroud. :(
> > > > I'm just looking if there can be other reasons to fail tests.
> > > > 
> > > > Because we introduced m_fetcher.clear() in detach(), some tests failed
> with
> > > > accessing nullptr'ed m_fetcher.
> > > 
> > > I can't see much going wrong when I revert those changes locally. I think
we
> > do
> > > need the one in Document::finishedParsing(), but that might be it?
> > > 
> > > fast/files/workers/worker-read-blob-async.html is failing though; but
that's
> > > also failing with the last trybot runs here.
> > 
> > Having looked at that test failure some, I'm wondering if the extra clearing
> of
> > ResourceFetchers added here needs to be reconsidered. i.e., a detached
> Document
> > ends up being used by a Worker to initiate a load -- finding no
> ResourceFetcher
> > when doing so.
> 
> Do you think the eager removal/clearing of the ResourceFetchers is
stable&viable
> overall?
> 
> We need to make forward progress.

Hmm, I think the eager removal is not stable overall, as far as a
Document/DocumentLoader
can be reused after detaching.
I mean, the lifetime of ResourceFetcher should be same with the lifetime of its
owners.

[sof, 2015-07-01 06:13:51]

On 2015/06/30 08:08:27, peria wrote:
> On 2015/06/30 07:39:40, sof wrote:
> > On 2015/06/29 15:00:47, sof wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
> > > File Source/core/loader/DocumentLoader.cpp (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
> > > Source/core/loader/DocumentLoader.cpp:647: if (m_fetcher)
> > > On 2015/06/29 13:29:29, sof wrote:
> > > > On 2015/06/26 07:31:43, peria wrote:
> > > > > On 2015/06/26 07:21:45, sof wrote:
> > > > > > Trying to understand; could you explain why the m_fetcher accesses
are
> > > null
> > > > > > checked now?
> > > > > 
> > > > > No theoretical backgroud. :(
> > > > > I'm just looking if there can be other reasons to fail tests.
> > > > > 
> > > > > Because we introduced m_fetcher.clear() in detach(), some tests failed
> > with
> > > > > accessing nullptr'ed m_fetcher.
> > > > 
> > > > I can't see much going wrong when I revert those changes locally. I
think
> we
> > > do
> > > > need the one in Document::finishedParsing(), but that might be it?
> > > > 
> > > > fast/files/workers/worker-read-blob-async.html is failing though; but
> that's
> > > > also failing with the last trybot runs here.
> > > 
> > > Having looked at that test failure some, I'm wondering if the extra
clearing
> > of
> > > ResourceFetchers added here needs to be reconsidered. i.e., a detached
> > Document
> > > ends up being used by a Worker to initiate a load -- finding no
> > ResourceFetcher
> > > when doing so.
> > 
> > Do you think the eager removal/clearing of the ResourceFetchers is
> stable&viable
> > overall?
> > 
> > We need to make forward progress.
> 
> Hmm, I think the eager removal is not stable overall, as far as a
> Document/DocumentLoader
> can be reused after detaching.
> I mean, the lifetime of ResourceFetcher should be same with the lifetime of
its
> owners.

Going back to https://codereview.chromium.org/1124153003/#msg35 (and earlier
comments), should we go after b) instead?

[peria, 2015-07-01 06:47:56]

On 2015/07/01 06:13:51, sof wrote:
> On 2015/06/30 08:08:27, peria wrote:
> > On 2015/06/30 07:39:40, sof wrote:
> > > On 2015/06/29 15:00:47, sof wrote:
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
> > > > File Source/core/loader/DocumentLoader.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
> > > > Source/core/loader/DocumentLoader.cpp:647: if (m_fetcher)
> > > > On 2015/06/29 13:29:29, sof wrote:
> > > > > On 2015/06/26 07:31:43, peria wrote:
> > > > > > On 2015/06/26 07:21:45, sof wrote:
> > > > > > > Trying to understand; could you explain why the m_fetcher accesses
> are
> > > > null
> > > > > > > checked now?
> > > > > > 
> > > > > > No theoretical backgroud. :(
> > > > > > I'm just looking if there can be other reasons to fail tests.
> > > > > > 
> > > > > > Because we introduced m_fetcher.clear() in detach(), some tests
failed
> > > with
> > > > > > accessing nullptr'ed m_fetcher.
> > > > > 
> > > > > I can't see much going wrong when I revert those changes locally. I
> think
> > we
> > > > do
> > > > > need the one in Document::finishedParsing(), but that might be it?
> > > > > 
> > > > > fast/files/workers/worker-read-blob-async.html is failing though; but
> > that's
> > > > > also failing with the last trybot runs here.
> > > > 
> > > > Having looked at that test failure some, I'm wondering if the extra
> clearing
> > > of
> > > > ResourceFetchers added here needs to be reconsidered. i.e., a detached
> > > Document
> > > > ends up being used by a Worker to initiate a load -- finding no
> > > ResourceFetcher
> > > > when doing so.
> > > 
> > > Do you think the eager removal/clearing of the ResourceFetchers is
> > stable&viable
> > > overall?
> > > 
> > > We need to make forward progress.
> > 
> > Hmm, I think the eager removal is not stable overall, as far as a
> > Document/DocumentLoader
> > can be reused after detaching.
> > I mean, the lifetime of ResourceFetcher should be same with the lifetime of
> its
> > owners.
> 
> Going back to https://codereview.chromium.org/1124153003/#msg35 (and earlier
> comments), should we go after b) instead?

Yes, I think we should.
I was focused on this issue too closely, but (b) in #35 meant that it is needed
to fix the leak independent from this migration.

[peria, 2015-07-01 07:44:53]

On 2015/07/01 06:47:56, peria wrote:
> On 2015/07/01 06:13:51, sof wrote:
> > On 2015/06/30 08:08:27, peria wrote:
> > > On 2015/06/30 07:39:40, sof wrote:
> > > > On 2015/06/29 15:00:47, sof wrote:
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
> > > > > File Source/core/loader/DocumentLoader.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
> > > > > Source/core/loader/DocumentLoader.cpp:647: if (m_fetcher)
> > > > > On 2015/06/29 13:29:29, sof wrote:
> > > > > > On 2015/06/26 07:31:43, peria wrote:
> > > > > > > On 2015/06/26 07:21:45, sof wrote:
> > > > > > > > Trying to understand; could you explain why the m_fetcher
accesses
> > are
> > > > > null
> > > > > > > > checked now?
> > > > > > > 
> > > > > > > No theoretical backgroud. :(
> > > > > > > I'm just looking if there can be other reasons to fail tests.
> > > > > > > 
> > > > > > > Because we introduced m_fetcher.clear() in detach(), some tests
> failed
> > > > with
> > > > > > > accessing nullptr'ed m_fetcher.
> > > > > > 
> > > > > > I can't see much going wrong when I revert those changes locally. I
> > think
> > > we
> > > > > do
> > > > > > need the one in Document::finishedParsing(), but that might be it?
> > > > > > 
> > > > > > fast/files/workers/worker-read-blob-async.html is failing though;
but
> > > that's
> > > > > > also failing with the last trybot runs here.
> > > > > 
> > > > > Having looked at that test failure some, I'm wondering if the extra
> > clearing
> > > > of
> > > > > ResourceFetchers added here needs to be reconsidered. i.e., a detached
> > > > Document
> > > > > ends up being used by a Worker to initiate a load -- finding no
> > > > ResourceFetcher
> > > > > when doing so.
> > > > 
> > > > Do you think the eager removal/clearing of the ResourceFetchers is
> > > stable&viable
> > > > overall?
> > > > 
> > > > We need to make forward progress.
> > > 
> > > Hmm, I think the eager removal is not stable overall, as far as a
> > > Document/DocumentLoader
> > > can be reused after detaching.
> > > I mean, the lifetime of ResourceFetcher should be same with the lifetime
of
> > its
> > > owners.
> > 
> > Going back to https://codereview.chromium.org/1124153003/#msg35 (and earlier
> > comments), should we go after b) instead?
> 
> Yes, I think we should.
> I was focused on this issue too closely, but (b) in #35 meant that it is
needed
> to fix the leak independent from this migration.

In addition, we can pass those tests separately.
I would like to file an issue to fix the leaks of Document, DocumentLoader,
and ResourceFetcher, and to progress this CL as a workaround.
WDYT?

[haraken, 2015-07-01 07:50:22]

On 2015/07/01 07:44:53, peria wrote:
> On 2015/07/01 06:47:56, peria wrote:
> > On 2015/07/01 06:13:51, sof wrote:
> > > On 2015/06/30 08:08:27, peria wrote:
> > > > On 2015/06/30 07:39:40, sof wrote:
> > > > > On 2015/06/29 15:00:47, sof wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
> > > > > > File Source/core/loader/DocumentLoader.cpp (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://chromiumcodereview.appspot.com/1124153003/diff/520001/Source/core/loa...
> > > > > > Source/core/loader/DocumentLoader.cpp:647: if (m_fetcher)
> > > > > > On 2015/06/29 13:29:29, sof wrote:
> > > > > > > On 2015/06/26 07:31:43, peria wrote:
> > > > > > > > On 2015/06/26 07:21:45, sof wrote:
> > > > > > > > > Trying to understand; could you explain why the m_fetcher
> accesses
> > > are
> > > > > > null
> > > > > > > > > checked now?
> > > > > > > > 
> > > > > > > > No theoretical backgroud. :(
> > > > > > > > I'm just looking if there can be other reasons to fail tests.
> > > > > > > > 
> > > > > > > > Because we introduced m_fetcher.clear() in detach(), some tests
> > failed
> > > > > with
> > > > > > > > accessing nullptr'ed m_fetcher.
> > > > > > > 
> > > > > > > I can't see much going wrong when I revert those changes locally.
I
> > > think
> > > > we
> > > > > > do
> > > > > > > need the one in Document::finishedParsing(), but that might be it?
> > > > > > > 
> > > > > > > fast/files/workers/worker-read-blob-async.html is failing though;
> but
> > > > that's
> > > > > > > also failing with the last trybot runs here.
> > > > > > 
> > > > > > Having looked at that test failure some, I'm wondering if the extra
> > > clearing
> > > > > of
> > > > > > ResourceFetchers added here needs to be reconsidered. i.e., a
detached
> > > > > Document
> > > > > > ends up being used by a Worker to initiate a load -- finding no
> > > > > ResourceFetcher
> > > > > > when doing so.
> > > > > 
> > > > > Do you think the eager removal/clearing of the ResourceFetchers is
> > > > stable&viable
> > > > > overall?
> > > > > 
> > > > > We need to make forward progress.
> > > > 
> > > > Hmm, I think the eager removal is not stable overall, as far as a
> > > > Document/DocumentLoader
> > > > can be reused after detaching.
> > > > I mean, the lifetime of ResourceFetcher should be same with the lifetime
> of
> > > its
> > > > owners.
> > > 
> > > Going back to https://codereview.chromium.org/1124153003/#msg35 (and
earlier
> > > comments), should we go after b) instead?
> > 
> > Yes, I think we should.
> > I was focused on this issue too closely, but (b) in #35 meant that it is
> needed
> > to fix the leak independent from this migration.
> 
> In addition, we can pass those tests separately.
> I would like to file an issue to fix the leaks of Document, DocumentLoader,
> and ResourceFetcher, and to progress this CL as a workaround.
> WDYT?

Sounds reasonable to me.

[peria, 2015-07-02 04:23:17]

> > > Going back to https://codereview.chromium.org/1124153003/#msg35 (and
earlier
> > > comments), should we go after b) instead?
> > 
> > Yes, I think we should.
> > I was focused on this issue too closely, but (b) in #35 meant that it is
> needed
> > to fix the leak independent from this migration.
> 
> In addition, we can pass those tests separately.
> I would like to file an issue to fix the leaks of Document, DocumentLoader,
> and ResourceFetcher, and to progress this CL as a workaround.
> WDYT?

Let me correct my comment.
As sof@ said in #55, we cannot clear m_fetcher in detaching
Document/DocumentLoader.
Current CL does it and passes all tests, but it goes well just in test cases.
Even if there is a workaround, it is not so easy.

[sof, 2015-07-02 05:24:48]

On 2015/07/02 04:23:17, peria wrote:
> > > > Going back to https://codereview.chromium.org/1124153003/#msg35 (and
> earlier
> > > > comments), should we go after b) instead?
> > > 
> > > Yes, I think we should.
> > > I was focused on this issue too closely, but (b) in #35 meant that it is
> > needed
> > > to fix the leak independent from this migration.
> > 
> > In addition, we can pass those tests separately.
> > I would like to file an issue to fix the leaks of Document, DocumentLoader,
> > and ResourceFetcher, and to progress this CL as a workaround.
> > WDYT?
> 
> Let me correct my comment.
> As sof@ said in #55, we cannot clear m_fetcher in detaching
> Document/DocumentLoader.
> Current CL does it and passes all tests, but it goes well just in test cases.
> Even if there is a workaround, it is not so easy.

It crashes on the fast/files/workers test still?

[haraken, 2015-07-02 05:26:45]

On 2015/07/02 04:23:17, peria wrote:
> > > > Going back to https://codereview.chromium.org/1124153003/#msg35 (and
> earlier
> > > > comments), should we go after b) instead?
> > > 
> > > Yes, I think we should.
> > > I was focused on this issue too closely, but (b) in #35 meant that it is
> > needed
> > > to fix the leak independent from this migration.
> > 
> > In addition, we can pass those tests separately.
> > I would like to file an issue to fix the leaks of Document, DocumentLoader,
> > and ResourceFetcher, and to progress this CL as a workaround.
> > WDYT?
> 
> Let me correct my comment.
> As sof@ said in #55, we cannot clear m_fetcher in detaching
> Document/DocumentLoader.
> Current CL does it and passes all tests, but it goes well just in test cases.
> Even if there is a workaround, it is not so easy.


> Having looked at that test failure some, I'm wondering if the extra clearing
of
> ResourceFetchers added here needs to be reconsidered. i.e., a detached
Document
> ends up being used by a Worker to initiate a load -- finding no
ResourceFetcher
> when doing so.

peria-san: Would you give me a stack trace where Worker tries to use a
ResourceFetcher of a detached document? Intuitively, it seems not to be a right
thing to use a thing in a detached document.

[peria, 2015-07-02 05:31:12]

On 2015/07/02 05:24:48, sof wrote:
> On 2015/07/02 04:23:17, peria wrote:
> > > > > Going back to https://codereview.chromium.org/1124153003/#msg35 (and
> > earlier
> > > > > comments), should we go after b) instead?
> > > > 
> > > > Yes, I think we should.
> > > > I was focused on this issue too closely, but (b) in #35 meant that it is
> > > needed
> > > > to fix the leak independent from this migration.
> > > 
> > > In addition, we can pass those tests separately.
> > > I would like to file an issue to fix the leaks of Document,
DocumentLoader,
> > > and ResourceFetcher, and to progress this CL as a workaround.
> > > WDYT?
> > 
> > Let me correct my comment.
> > As sof@ said in #55, we cannot clear m_fetcher in detaching
> > Document/DocumentLoader.
> > Current CL does it and passes all tests, but it goes well just in test
cases.
> > Even if there is a workaround, it is not so easy.
> 
> It crashes on the fast/files/workers test still?

Yes it crashes, but it passes the same test in the second try.

[peria, 2015-07-02 05:41:40]

On 2015/07/02 05:26:45, haraken wrote:
> On 2015/07/02 04:23:17, peria wrote:
> > > > > Going back to https://codereview.chromium.org/1124153003/#msg35 (and
> > earlier
> > > > > comments), should we go after b) instead?
> > > > 
> > > > Yes, I think we should.
> > > > I was focused on this issue too closely, but (b) in #35 meant that it is
> > > needed
> > > > to fix the leak independent from this migration.
> > > 
> > > In addition, we can pass those tests separately.
> > > I would like to file an issue to fix the leaks of Document,
DocumentLoader,
> > > and ResourceFetcher, and to progress this CL as a workaround.
> > > WDYT?
> > 
> > Let me correct my comment.
> > As sof@ said in #55, we cannot clear m_fetcher in detaching
> > Document/DocumentLoader.
> > Current CL does it and passes all tests, but it goes well just in test
cases.
> > Even if there is a workaround, it is not so easy.
> 
> 
> > Having looked at that test failure some, I'm wondering if the extra clearing
> of
> > ResourceFetchers added here needs to be reconsidered. i.e., a detached
> Document
> > ends up being used by a Worker to initiate a load -- finding no
> ResourceFetcher
> > when doing so.
> 
> peria-san: Would you give me a stack trace where Worker tries to use a
> ResourceFetcher of a detached document? Intuitively, it seems not to be a
right
> thing to use a thing in a detached document.

I have no practical data to do such behaviors.
I meant that this CL was not a solution, if a detached Document/DocumentLoader
could be reused.
If reusing a detached Document is invalid for everyone, I think this CL is ready
to be reviewed.

[haraken, 2015-07-02 05:43:20]

On 2015/07/02 05:41:40, peria wrote:
> On 2015/07/02 05:26:45, haraken wrote:
> > On 2015/07/02 04:23:17, peria wrote:
> > > > > > Going back to https://codereview.chromium.org/1124153003/#msg35 (and
> > > earlier
> > > > > > comments), should we go after b) instead?
> > > > > 
> > > > > Yes, I think we should.
> > > > > I was focused on this issue too closely, but (b) in #35 meant that it
is
> > > > needed
> > > > > to fix the leak independent from this migration.
> > > > 
> > > > In addition, we can pass those tests separately.
> > > > I would like to file an issue to fix the leaks of Document,
> DocumentLoader,
> > > > and ResourceFetcher, and to progress this CL as a workaround.
> > > > WDYT?
> > > 
> > > Let me correct my comment.
> > > As sof@ said in #55, we cannot clear m_fetcher in detaching
> > > Document/DocumentLoader.
> > > Current CL does it and passes all tests, but it goes well just in test
> cases.
> > > Even if there is a workaround, it is not so easy.
> > 
> > 
> > > Having looked at that test failure some, I'm wondering if the extra
clearing
> > of
> > > ResourceFetchers added here needs to be reconsidered. i.e., a detached
> > Document
> > > ends up being used by a Worker to initiate a load -- finding no
> > ResourceFetcher
> > > when doing so.
> > 
> > peria-san: Would you give me a stack trace where Worker tries to use a
> > ResourceFetcher of a detached document? Intuitively, it seems not to be a
> right
> > thing to use a thing in a detached document.
> 
> I have no practical data to do such behaviors.
> I meant that this CL was not a solution, if a detached Document/DocumentLoader
> could be reused.
> If reusing a detached Document is invalid for everyone, I think this CL is
ready
> to be reviewed.

Conceptually it should not happen. If it happens, I think that needs to be
fixed.

Can you get a stack trace of the first try that crashes the worker?

[sof, 2015-07-02 06:06:55]

On 2015/07/02 05:43:20, haraken wrote:
> On 2015/07/02 05:41:40, peria wrote:
> > On 2015/07/02 05:26:45, haraken wrote:
> > > On 2015/07/02 04:23:17, peria wrote:
> > > > > > > Going back to https://codereview.chromium.org/1124153003/#msg35
(and
> > > > earlier
> > > > > > > comments), should we go after b) instead?
> > > > > > 
> > > > > > Yes, I think we should.
> > > > > > I was focused on this issue too closely, but (b) in #35 meant that
it
> is
> > > > > needed
> > > > > > to fix the leak independent from this migration.
> > > > > 
> > > > > In addition, we can pass those tests separately.
> > > > > I would like to file an issue to fix the leaks of Document,
> > DocumentLoader,
> > > > > and ResourceFetcher, and to progress this CL as a workaround.
> > > > > WDYT?
> > > > 
> > > > Let me correct my comment.
> > > > As sof@ said in #55, we cannot clear m_fetcher in detaching
> > > > Document/DocumentLoader.
> > > > Current CL does it and passes all tests, but it goes well just in test
> > cases.
> > > > Even if there is a workaround, it is not so easy.
> > > 
> > > 
> > > > Having looked at that test failure some, I'm wondering if the extra
> clearing
> > > of
> > > > ResourceFetchers added here needs to be reconsidered. i.e., a detached
> > > Document
> > > > ends up being used by a Worker to initiate a load -- finding no
> > > ResourceFetcher
> > > > when doing so.
> > > 
> > > peria-san: Would you give me a stack trace where Worker tries to use a
> > > ResourceFetcher of a detached document? Intuitively, it seems not to be a
> > right
> > > thing to use a thing in a detached document.
> > 
> > I have no practical data to do such behaviors.
> > I meant that this CL was not a solution, if a detached
Document/DocumentLoader
> > could be reused.
> > If reusing a detached Document is invalid for everyone, I think this CL is
> ready
> > to be reviewed.
> 
> Conceptually it should not happen. If it happens, I think that needs to be
> fixed.
> 
> Can you get a stack trace of the first try that crashes the worker?

This is crazy. The CL adds a bunch of null checks to try to work around
that..and it doesn't work.

ResourceFetcher and its handling is tricky stuff; taking on risky changes in
addition here seems counter-productive.

[peria, 2015-07-02 06:14:17]

On 2015/07/02 05:43:20, haraken wrote:
> On 2015/07/02 05:41:40, peria wrote:
> > On 2015/07/02 05:26:45, haraken wrote:
> > > On 2015/07/02 04:23:17, peria wrote:
> > > > > > > Going back to https://codereview.chromium.org/1124153003/#msg35
(and
> > > > earlier
> > > > > > > comments), should we go after b) instead?
> > > > > > 
> > > > > > Yes, I think we should.
> > > > > > I was focused on this issue too closely, but (b) in #35 meant that
it
> is
> > > > > needed
> > > > > > to fix the leak independent from this migration.
> > > > > 
> > > > > In addition, we can pass those tests separately.
> > > > > I would like to file an issue to fix the leaks of Document,
> > DocumentLoader,
> > > > > and ResourceFetcher, and to progress this CL as a workaround.
> > > > > WDYT?
> > > > 
> > > > Let me correct my comment.
> > > > As sof@ said in #55, we cannot clear m_fetcher in detaching
> > > > Document/DocumentLoader.
> > > > Current CL does it and passes all tests, but it goes well just in test
> > cases.
> > > > Even if there is a workaround, it is not so easy.
> > > 
> > > 
> > > > Having looked at that test failure some, I'm wondering if the extra
> clearing
> > > of
> > > > ResourceFetchers added here needs to be reconsidered. i.e., a detached
> > > Document
> > > > ends up being used by a Worker to initiate a load -- finding no
> > > ResourceFetcher
> > > > when doing so.
> > > 
> > > peria-san: Would you give me a stack trace where Worker tries to use a
> > > ResourceFetcher of a detached document? Intuitively, it seems not to be a
> > right
> > > thing to use a thing in a detached document.
> > 
> > I have no practical data to do such behaviors.
> > I meant that this CL was not a solution, if a detached
Document/DocumentLoader
> > could be reused.
> > If reusing a detached Document is invalid for everyone, I think this CL is
> ready
> > to be reviewed.
> 
> Conceptually it should not happen. If it happens, I think that needs to be
> fixed.
> 
> Can you get a stack trace of the first try that crashes the worker?

Hmm, I tried but could not reproduce the crash on my machine.

[peria, 2015-07-02 06:18:26]

> This is crazy. The CL adds a bunch of null checks to try to work around
> that..and it doesn't work.
I added some null checks to pass tests, but I think some of them are
really required and others are just for tests.
I'll work to drop latter cases.

[sof, 2015-07-02 06:26:53]

On 2015/07/02 06:14:17, peria wrote:
> On 2015/07/02 05:43:20, haraken wrote:
> > On 2015/07/02 05:41:40, peria wrote:
> > > On 2015/07/02 05:26:45, haraken wrote:
> > > > On 2015/07/02 04:23:17, peria wrote:
> > > > > > > > Going back to https://codereview.chromium.org/1124153003/#msg35
> (and
> > > > > earlier
> > > > > > > > comments), should we go after b) instead?
> > > > > > > 
> > > > > > > Yes, I think we should.
> > > > > > > I was focused on this issue too closely, but (b) in #35 meant that
> it
> > is
> > > > > > needed
> > > > > > > to fix the leak independent from this migration.
> > > > > > 
> > > > > > In addition, we can pass those tests separately.
> > > > > > I would like to file an issue to fix the leaks of Document,
> > > DocumentLoader,
> > > > > > and ResourceFetcher, and to progress this CL as a workaround.
> > > > > > WDYT?
> > > > > 
> > > > > Let me correct my comment.
> > > > > As sof@ said in #55, we cannot clear m_fetcher in detaching
> > > > > Document/DocumentLoader.
> > > > > Current CL does it and passes all tests, but it goes well just in test
> > > cases.
> > > > > Even if there is a workaround, it is not so easy.
> > > > 
> > > > 
> > > > > Having looked at that test failure some, I'm wondering if the extra
> > clearing
> > > > of
> > > > > ResourceFetchers added here needs to be reconsidered. i.e., a detached
> > > > Document
> > > > > ends up being used by a Worker to initiate a load -- finding no
> > > > ResourceFetcher
> > > > > when doing so.
> > > > 
> > > > peria-san: Would you give me a stack trace where Worker tries to use a
> > > > ResourceFetcher of a detached document? Intuitively, it seems not to be
a
> > > right
> > > > thing to use a thing in a detached document.
> > > 
> > > I have no practical data to do such behaviors.
> > > I meant that this CL was not a solution, if a detached
> Document/DocumentLoader
> > > could be reused.
> > > If reusing a detached Document is invalid for everyone, I think this CL is
> > ready
> > > to be reviewed.
> > 
> > Conceptually it should not happen. If it happens, I think that needs to be
> > fixed.
> > 
> > Can you get a stack trace of the first try that crashes the worker?
> 
> Hmm, I tried but could not reproduce the crash on my machine.

Here's the stack trace at the point of failure,

STDERR: Received signal 11 SEGV_MAPERR 000000000008
STDERR: #0 0x0000007d3ace base::debug::StackTrace::StackTrace()
STDERR: #1 0x0000007d360f base::debug::(anonymous
namespace)::StackDumpSignalHandler()
STDERR: #2 0x7f6449239340 <unknown>
STDERR: #3 0x00000043842c base::internal::scoped_ptr_impl<>::get()
STDERR: #4 0x0000027f3503 blink::ResourceFetcher::context()
STDERR: #5 0x000002ee2039 blink::ResourceFetcher::requestResource()
STDERR: #6 0x000002ed2f76 blink::RawResource::fetch()
STDERR: #7 0x000003066457 blink::DocumentThreadableLoader::loadRequest()
STDERR: #8 0x0000030669f0
blink::DocumentThreadableLoader::dispatchInitialRequest()
STDERR: #9 0x000003065e37
blink::DocumentThreadableLoader::DocumentThreadableLoader()
STDERR: #10 0x000003065719 blink::DocumentThreadableLoader::create()
STDERR: #11 0x00000309056a
blink::WorkerThreadableLoader::MainThreadBridge::mainThreadCreateLoader()
STDERR: #12 0x000003092400 WTF::FunctionWrapper<>::operator()()
STDERR: #13 0x000003092191 WTF::PartBoundFunctionImpl<>::operator()()
STDERR: #14 0x0000026836c3
blink::internal::CallClosureWithExecutionContextTask::performTask()
STDERR: #15 0x0000028516bb blink::MainThreadTaskRunner::perform()
STDERR: #16 0x000002851574 blink::MainThreadTask::run()
STDERR: #17 0x000004b9bf9e scheduler::WebThreadBase::RunWebThreadTask()
STDERR: #18 0x000004b9b7aa base::internal::RunnableAdapter<>::Run()

[haraken, 2015-07-02 06:37:24]

On 2015/07/02 06:06:55, sof wrote:
> On 2015/07/02 05:43:20, haraken wrote:
> > On 2015/07/02 05:41:40, peria wrote:
> > > On 2015/07/02 05:26:45, haraken wrote:
> > > > On 2015/07/02 04:23:17, peria wrote:
> > > > > > > > Going back to https://codereview.chromium.org/1124153003/#msg35
> (and
> > > > > earlier
> > > > > > > > comments), should we go after b) instead?
> > > > > > > 
> > > > > > > Yes, I think we should.
> > > > > > > I was focused on this issue too closely, but (b) in #35 meant that
> it
> > is
> > > > > > needed
> > > > > > > to fix the leak independent from this migration.
> > > > > > 
> > > > > > In addition, we can pass those tests separately.
> > > > > > I would like to file an issue to fix the leaks of Document,
> > > DocumentLoader,
> > > > > > and ResourceFetcher, and to progress this CL as a workaround.
> > > > > > WDYT?
> > > > > 
> > > > > Let me correct my comment.
> > > > > As sof@ said in #55, we cannot clear m_fetcher in detaching
> > > > > Document/DocumentLoader.
> > > > > Current CL does it and passes all tests, but it goes well just in test
> > > cases.
> > > > > Even if there is a workaround, it is not so easy.
> > > > 
> > > > 
> > > > > Having looked at that test failure some, I'm wondering if the extra
> > clearing
> > > > of
> > > > > ResourceFetchers added here needs to be reconsidered. i.e., a detached
> > > > Document
> > > > > ends up being used by a Worker to initiate a load -- finding no
> > > > ResourceFetcher
> > > > > when doing so.
> > > > 
> > > > peria-san: Would you give me a stack trace where Worker tries to use a
> > > > ResourceFetcher of a detached document? Intuitively, it seems not to be
a
> > > right
> > > > thing to use a thing in a detached document.
> > > 
> > > I have no practical data to do such behaviors.
> > > I meant that this CL was not a solution, if a detached
> Document/DocumentLoader
> > > could be reused.
> > > If reusing a detached Document is invalid for everyone, I think this CL is
> > ready
> > > to be reviewed.
> > 
> > Conceptually it should not happen. If it happens, I think that needs to be
> > fixed.
> > 
> > Can you get a stack trace of the first try that crashes the worker?
> 
> This is crazy. The CL adds a bunch of null checks to try to work around
> that..and it doesn't work.
> 
> ResourceFetcher and its handling is tricky stuff; taking on risky changes in
> addition here seems counter-productive.

Just to clarify: I'm not necessarily proposing to clear ResourceFetcher in
Document::detach. It is not a right thing to insert null checks without
understanding what you're doing. What I'm proposing is to do what makes the best
sense here.

If it is not easy or is risky to clear ResourceFetcher in Document::detach, it
makes sense to land this CL with suppressing the LSan leak. Otherwise, we should
clear ResourceFetcher in Document::detach and fix the code accordingly. That's
why I asked to get a stack trace of the worker crashes.

peria-san: What's your conclusion?

[haraken, 2015-07-02 06:39:59]

On 2015/07/02 06:26:53, sof wrote:
> On 2015/07/02 06:14:17, peria wrote:
> > On 2015/07/02 05:43:20, haraken wrote:
> > > On 2015/07/02 05:41:40, peria wrote:
> > > > On 2015/07/02 05:26:45, haraken wrote:
> > > > > On 2015/07/02 04:23:17, peria wrote:
> > > > > > > > > Going back to
https://codereview.chromium.org/1124153003/#msg35
> > (and
> > > > > > earlier
> > > > > > > > > comments), should we go after b) instead?
> > > > > > > > 
> > > > > > > > Yes, I think we should.
> > > > > > > > I was focused on this issue too closely, but (b) in #35 meant
that
> > it
> > > is
> > > > > > > needed
> > > > > > > > to fix the leak independent from this migration.
> > > > > > > 
> > > > > > > In addition, we can pass those tests separately.
> > > > > > > I would like to file an issue to fix the leaks of Document,
> > > > DocumentLoader,
> > > > > > > and ResourceFetcher, and to progress this CL as a workaround.
> > > > > > > WDYT?
> > > > > > 
> > > > > > Let me correct my comment.
> > > > > > As sof@ said in #55, we cannot clear m_fetcher in detaching
> > > > > > Document/DocumentLoader.
> > > > > > Current CL does it and passes all tests, but it goes well just in
test
> > > > cases.
> > > > > > Even if there is a workaround, it is not so easy.
> > > > > 
> > > > > 
> > > > > > Having looked at that test failure some, I'm wondering if the extra
> > > clearing
> > > > > of
> > > > > > ResourceFetchers added here needs to be reconsidered. i.e., a
detached
> > > > > Document
> > > > > > ends up being used by a Worker to initiate a load -- finding no
> > > > > ResourceFetcher
> > > > > > when doing so.
> > > > > 
> > > > > peria-san: Would you give me a stack trace where Worker tries to use a
> > > > > ResourceFetcher of a detached document? Intuitively, it seems not to
be
> a
> > > > right
> > > > > thing to use a thing in a detached document.
> > > > 
> > > > I have no practical data to do such behaviors.
> > > > I meant that this CL was not a solution, if a detached
> > Document/DocumentLoader
> > > > could be reused.
> > > > If reusing a detached Document is invalid for everyone, I think this CL
is
> > > ready
> > > > to be reviewed.
> > > 
> > > Conceptually it should not happen. If it happens, I think that needs to be
> > > fixed.
> > > 
> > > Can you get a stack trace of the first try that crashes the worker?
> > 
> > Hmm, I tried but could not reproduce the crash on my machine.
> 
> Here's the stack trace at the point of failure,
> 
> STDERR: Received signal 11 SEGV_MAPERR 000000000008
> STDERR: #0 0x0000007d3ace base::debug::StackTrace::StackTrace()
> STDERR: #1 0x0000007d360f base::debug::(anonymous
> namespace)::StackDumpSignalHandler()
> STDERR: #2 0x7f6449239340 <unknown>
> STDERR: #3 0x00000043842c base::internal::scoped_ptr_impl<>::get()
> STDERR: #4 0x0000027f3503 blink::ResourceFetcher::context()
> STDERR: #5 0x000002ee2039 blink::ResourceFetcher::requestResource()
> STDERR: #6 0x000002ed2f76 blink::RawResource::fetch()
> STDERR: #7 0x000003066457 blink::DocumentThreadableLoader::loadRequest()
> STDERR: #8 0x0000030669f0
> blink::DocumentThreadableLoader::dispatchInitialRequest()
> STDERR: #9 0x000003065e37
> blink::DocumentThreadableLoader::DocumentThreadableLoader()
> STDERR: #10 0x000003065719 blink::DocumentThreadableLoader::create()
> STDERR: #11 0x00000309056a
> blink::WorkerThreadableLoader::MainThreadBridge::mainThreadCreateLoader()
> STDERR: #12 0x000003092400 WTF::FunctionWrapper<>::operator()()
> STDERR: #13 0x000003092191 WTF::PartBoundFunctionImpl<>::operator()()
> STDERR: #14 0x0000026836c3
> blink::internal::CallClosureWithExecutionContextTask::performTask()
> STDERR: #15 0x0000028516bb blink::MainThreadTaskRunner::perform()
> STDERR: #16 0x000002851574 blink::MainThreadTask::run()
> STDERR: #17 0x000004b9bf9e scheduler::WebThreadBase::RunWebThreadTask()
> STDERR: #18 0x000004b9b7aa base::internal::RunnableAdapter<>::Run()

Thanks!

I'd argue that DocumentThreadableLoader::create should return 0 if the document
is already detached, but that wouldn't be a trivial change.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[peria, 2015-07-02 06:55:46]

On 2015/07/02 06:37:24, haraken wrote:
> On 2015/07/02 06:06:55, sof wrote:
> > On 2015/07/02 05:43:20, haraken wrote:
> > > On 2015/07/02 05:41:40, peria wrote:
> > > > On 2015/07/02 05:26:45, haraken wrote:
> > > > > On 2015/07/02 04:23:17, peria wrote:
> > > > > > > > > Going back to
https://codereview.chromium.org/1124153003/#msg35
> > (and
> > > > > > earlier
> > > > > > > > > comments), should we go after b) instead?
> > > > > > > > 
> > > > > > > > Yes, I think we should.
> > > > > > > > I was focused on this issue too closely, but (b) in #35 meant
that
> > it
> > > is
> > > > > > > needed
> > > > > > > > to fix the leak independent from this migration.
> > > > > > > 
> > > > > > > In addition, we can pass those tests separately.
> > > > > > > I would like to file an issue to fix the leaks of Document,
> > > > DocumentLoader,
> > > > > > > and ResourceFetcher, and to progress this CL as a workaround.
> > > > > > > WDYT?
> > > > > > 
> > > > > > Let me correct my comment.
> > > > > > As sof@ said in #55, we cannot clear m_fetcher in detaching
> > > > > > Document/DocumentLoader.
> > > > > > Current CL does it and passes all tests, but it goes well just in
test
> > > > cases.
> > > > > > Even if there is a workaround, it is not so easy.
> > > > > 
> > > > > 
> > > > > > Having looked at that test failure some, I'm wondering if the extra
> > > clearing
> > > > > of
> > > > > > ResourceFetchers added here needs to be reconsidered. i.e., a
detached
> > > > > Document
> > > > > > ends up being used by a Worker to initiate a load -- finding no
> > > > > ResourceFetcher
> > > > > > when doing so.
> > > > > 
> > > > > peria-san: Would you give me a stack trace where Worker tries to use a
> > > > > ResourceFetcher of a detached document? Intuitively, it seems not to
be
> a
> > > > right
> > > > > thing to use a thing in a detached document.
> > > > 
> > > > I have no practical data to do such behaviors.
> > > > I meant that this CL was not a solution, if a detached
> > Document/DocumentLoader
> > > > could be reused.
> > > > If reusing a detached Document is invalid for everyone, I think this CL
is
> > > ready
> > > > to be reviewed.
> > > 
> > > Conceptually it should not happen. If it happens, I think that needs to be
> > > fixed.
> > > 
> > > Can you get a stack trace of the first try that crashes the worker?
> > 
> > This is crazy. The CL adds a bunch of null checks to try to work around
> > that..and it doesn't work.
> > 
> > ResourceFetcher and its handling is tricky stuff; taking on risky changes in
> > addition here seems counter-productive.
> 
> Just to clarify: I'm not necessarily proposing to clear ResourceFetcher in
> Document::detach. It is not a right thing to insert null checks without
> understanding what you're doing. What I'm proposing is to do what makes the
best
> sense here.
> 
> If it is not easy or is risky to clear ResourceFetcher in Document::detach, it
> makes sense to land this CL with suppressing the LSan leak. Otherwise, we
should
> clear ResourceFetcher in Document::detach and fix the code accordingly. That's
> why I asked to get a stack trace of the worker crashes.
> 
> peria-san: What's your conclusion?

IMHO, this CL should not add m_fetch.clear() in detach(), and we need to fix
the leaks of Document/DocumentLoader separately.

The reason is as following
(1) Document and DocumentLoader have already been leaked, independent from this
CL,
(2) Clearing pointers of ResourceFetcher is just a workaround and it is not
safe.

[haraken, 2015-07-02 06:57:57]

On 2015/07/02 06:55:46, peria wrote:
> On 2015/07/02 06:37:24, haraken wrote:
> > On 2015/07/02 06:06:55, sof wrote:
> > > On 2015/07/02 05:43:20, haraken wrote:
> > > > On 2015/07/02 05:41:40, peria wrote:
> > > > > On 2015/07/02 05:26:45, haraken wrote:
> > > > > > On 2015/07/02 04:23:17, peria wrote:
> > > > > > > > > > Going back to
> https://codereview.chromium.org/1124153003/#msg35
> > > (and
> > > > > > > earlier
> > > > > > > > > > comments), should we go after b) instead?
> > > > > > > > > 
> > > > > > > > > Yes, I think we should.
> > > > > > > > > I was focused on this issue too closely, but (b) in #35 meant
> that
> > > it
> > > > is
> > > > > > > > needed
> > > > > > > > > to fix the leak independent from this migration.
> > > > > > > > 
> > > > > > > > In addition, we can pass those tests separately.
> > > > > > > > I would like to file an issue to fix the leaks of Document,
> > > > > DocumentLoader,
> > > > > > > > and ResourceFetcher, and to progress this CL as a workaround.
> > > > > > > > WDYT?
> > > > > > > 
> > > > > > > Let me correct my comment.
> > > > > > > As sof@ said in #55, we cannot clear m_fetcher in detaching
> > > > > > > Document/DocumentLoader.
> > > > > > > Current CL does it and passes all tests, but it goes well just in
> test
> > > > > cases.
> > > > > > > Even if there is a workaround, it is not so easy.
> > > > > > 
> > > > > > 
> > > > > > > Having looked at that test failure some, I'm wondering if the
extra
> > > > clearing
> > > > > > of
> > > > > > > ResourceFetchers added here needs to be reconsidered. i.e., a
> detached
> > > > > > Document
> > > > > > > ends up being used by a Worker to initiate a load -- finding no
> > > > > > ResourceFetcher
> > > > > > > when doing so.
> > > > > > 
> > > > > > peria-san: Would you give me a stack trace where Worker tries to use
a
> > > > > > ResourceFetcher of a detached document? Intuitively, it seems not to
> be
> > a
> > > > > right
> > > > > > thing to use a thing in a detached document.
> > > > > 
> > > > > I have no practical data to do such behaviors.
> > > > > I meant that this CL was not a solution, if a detached
> > > Document/DocumentLoader
> > > > > could be reused.
> > > > > If reusing a detached Document is invalid for everyone, I think this
CL
> is
> > > > ready
> > > > > to be reviewed.
> > > > 
> > > > Conceptually it should not happen. If it happens, I think that needs to
be
> > > > fixed.
> > > > 
> > > > Can you get a stack trace of the first try that crashes the worker?
> > > 
> > > This is crazy. The CL adds a bunch of null checks to try to work around
> > > that..and it doesn't work.
> > > 
> > > ResourceFetcher and its handling is tricky stuff; taking on risky changes
in
> > > addition here seems counter-productive.
> > 
> > Just to clarify: I'm not necessarily proposing to clear ResourceFetcher in
> > Document::detach. It is not a right thing to insert null checks without
> > understanding what you're doing. What I'm proposing is to do what makes the
> best
> > sense here.
> > 
> > If it is not easy or is risky to clear ResourceFetcher in Document::detach,
it
> > makes sense to land this CL with suppressing the LSan leak. Otherwise, we
> should
> > clear ResourceFetcher in Document::detach and fix the code accordingly.
That's
> > why I asked to get a stack trace of the worker crashes.
> > 
> > peria-san: What's your conclusion?
> 
> IMHO, this CL should not add m_fetch.clear() in detach(), and we need to fix
> the leaks of Document/DocumentLoader separately.
> 
> The reason is as following
> (1) Document and DocumentLoader have already been leaked, independent from
this
> CL,
> (2) Clearing pointers of ResourceFetcher is just a workaround and it is not
> safe.

Sounds like a reasonable decision to me.

[peria, 2015-07-02 07:03:27]

On 2015/07/02 06:55:46, peria wrote:
> 
> IMHO, this CL should not add m_fetch.clear() in detach(), and we need to fix
> the leaks of Document/DocumentLoader separately.
> 
> The reason is as following
> (1) Document and DocumentLoader have already been leaked, independent from
this
> CL,
> (2) Clearing pointers of ResourceFetcher is just a workaround and it is not
> safe.

I'm sorry to change directions for many times, and took very long time to work
for them.
I would like to go as I said above.
Sigbjorn, what do you think?

[sof, 2015-07-02 08:19:36]

On 2015/07/02 07:03:27, peria wrote:
> On 2015/07/02 06:55:46, peria wrote:
> > 
> > IMHO, this CL should not add m_fetch.clear() in detach(), and we need to fix
> > the leaks of Document/DocumentLoader separately.
> > 
> > The reason is as following
> > (1) Document and DocumentLoader have already been leaked, independent from
> this
> > CL,
> > (2) Clearing pointers of ResourceFetcher is just a workaround and it is not
> > safe.
> 
> I'm sorry to change directions for many times, and took very long time to work
> for them.
> I would like to go as I said above.
> Sigbjorn, what do you think?

It is temptingly to take the other route, but I think that addressing the test
leaks is the workable way to go. Two reasons:

 - ResourceFetcher doesn't have a well-defined lifetime wrt Document.
 - When/if other Persistent<>s are temporarily added to Document later on, we'd
have to address the clearing of those too in a timely fashion.

[peria, 2015-07-02 08:49:46]

On 2015/07/02 08:19:36, sof wrote:
> On 2015/07/02 07:03:27, peria wrote:
> > On 2015/07/02 06:55:46, peria wrote:
> > > 
> > > IMHO, this CL should not add m_fetch.clear() in detach(), and we need to
fix
> > > the leaks of Document/DocumentLoader separately.
> > > 
> > > The reason is as following
> > > (1) Document and DocumentLoader have already been leaked, independent from
> > this
> > > CL,
> > > (2) Clearing pointers of ResourceFetcher is just a workaround and it is
not
> > > safe.
> > 
> > I'm sorry to change directions for many times, and took very long time to
work
> > for them.
> > I would like to go as I said above.
> > Sigbjorn, what do you think?
> 
> It is temptingly to take the other route, but I think that addressing the test
> leaks is the workable way to go. Two reasons:
> 
>  - ResourceFetcher doesn't have a well-defined lifetime wrt Document.
>  - When/if other Persistent<>s are temporarily added to Document later on,
we'd
> have to address the clearing of those too in a timely fashion.

Let me confirm.  You mean
1) we should add m_fetch.clear() in detach()
2) test failures should be fixed in reasonable ways
Is my understanding correct?

[sof, 2015-07-02 09:00:38]

On 2015/07/02 08:49:46, peria wrote:
> On 2015/07/02 08:19:36, sof wrote:
> > On 2015/07/02 07:03:27, peria wrote:
> > > On 2015/07/02 06:55:46, peria wrote:
> > > > 
> > > > IMHO, this CL should not add m_fetch.clear() in detach(), and we need to
> fix
> > > > the leaks of Document/DocumentLoader separately.
> > > > 
> > > > The reason is as following
> > > > (1) Document and DocumentLoader have already been leaked, independent
from
> > > this
> > > > CL,
> > > > (2) Clearing pointers of ResourceFetcher is just a workaround and it is
> not
> > > > safe.
> > > 
> > > I'm sorry to change directions for many times, and took very long time to
> work
> > > for them.
> > > I would like to go as I said above.
> > > Sigbjorn, what do you think?
> > 
> > It is temptingly to take the other route, but I think that addressing the
test
> > leaks is the workable way to go. Two reasons:
> > 
> >  - ResourceFetcher doesn't have a well-defined lifetime wrt Document.
> >  - When/if other Persistent<>s are temporarily added to Document later on,
> we'd
> > have to address the clearing of those too in a timely fashion.
> 
> Let me confirm.  You mean
> 1) we should add m_fetch.clear() in detach()
> 2) test failures should be fixed in reasonable ways
> Is my understanding correct?

No, address content_browsertests _leaks_ reported by lsan not _failures_ from
doing this clearing. i.e., have
https://codereview.chromium.org/1124153003/#msg24 pass.

[peria, 2015-07-02 09:22:40]

On 2015/07/02 09:00:38, sof wrote:
> On 2015/07/02 08:49:46, peria wrote:
> > On 2015/07/02 08:19:36, sof wrote:
> > > On 2015/07/02 07:03:27, peria wrote:
> > > > On 2015/07/02 06:55:46, peria wrote:
> > > > > 
> > > > > IMHO, this CL should not add m_fetch.clear() in detach(), and we need
to
> > fix
> > > > > the leaks of Document/DocumentLoader separately.
> > > > > 
> > > > > The reason is as following
> > > > > (1) Document and DocumentLoader have already been leaked, independent
> from
> > > > this
> > > > > CL,
> > > > > (2) Clearing pointers of ResourceFetcher is just a workaround and it
is
> > not
> > > > > safe.
> > > > 
> > > > I'm sorry to change directions for many times, and took very long time
to
> > work
> > > > for them.
> > > > I would like to go as I said above.
> > > > Sigbjorn, what do you think?
> > > 
> > > It is temptingly to take the other route, but I think that addressing the
> test
> > > leaks is the workable way to go. Two reasons:
> > > 
> > >  - ResourceFetcher doesn't have a well-defined lifetime wrt Document.
> > >  - When/if other Persistent<>s are temporarily added to Document later on,
> > we'd
> > > have to address the clearing of those too in a timely fashion.
> > 
> > Let me confirm.  You mean
> > 1) we should add m_fetch.clear() in detach()
> > 2) test failures should be fixed in reasonable ways
> > Is my understanding correct?
> 
> No, address content_browsertests _leaks_ reported by lsan not _failures_ from
> doing this clearing. i.e., have
> https://codereview.chromium.org/1124153003/#msg24 pass.

Simply, you meant "do the Oilpan migration, and fix the leaks", right?

[sof, 2015-07-02 09:29:28]

On 2015/07/02 09:22:40, peria wrote:
> On 2015/07/02 09:00:38, sof wrote:
> > On 2015/07/02 08:49:46, peria wrote:
> > > On 2015/07/02 08:19:36, sof wrote:
> > > > On 2015/07/02 07:03:27, peria wrote:
> > > > > On 2015/07/02 06:55:46, peria wrote:
> > > > > > 
> > > > > > IMHO, this CL should not add m_fetch.clear() in detach(), and we
need
> to
> > > fix
> > > > > > the leaks of Document/DocumentLoader separately.
> > > > > > 
> > > > > > The reason is as following
> > > > > > (1) Document and DocumentLoader have already been leaked,
independent
> > from
> > > > > this
> > > > > > CL,
> > > > > > (2) Clearing pointers of ResourceFetcher is just a workaround and it
> is
> > > not
> > > > > > safe.
> > > > > 
> > > > > I'm sorry to change directions for many times, and took very long time
> to
> > > work
> > > > > for them.
> > > > > I would like to go as I said above.
> > > > > Sigbjorn, what do you think?
> > > > 
> > > > It is temptingly to take the other route, but I think that addressing
the
> > test
> > > > leaks is the workable way to go. Two reasons:
> > > > 
> > > >  - ResourceFetcher doesn't have a well-defined lifetime wrt Document.
> > > >  - When/if other Persistent<>s are temporarily added to Document later
on,
> > > we'd
> > > > have to address the clearing of those too in a timely fashion.
> > > 
> > > Let me confirm.  You mean
> > > 1) we should add m_fetch.clear() in detach()
> > > 2) test failures should be fixed in reasonable ways
> > > Is my understanding correct?
> > 
> > No, address content_browsertests _leaks_ reported by lsan not _failures_
from
> > doing this clearing. i.e., have
> > https://codereview.chromium.org/1124153003/#msg24 pass.
> 
> Simply, you meant "do the Oilpan migration, and fix the leaks", right?

Yes.

[peria, 2015-07-03 02:37:39]

On 2015/07/02 09:29:28, sof(OOO) wrote:
> On 2015/07/02 09:22:40, peria wrote:
> > On 2015/07/02 09:00:38, sof wrote:
> > > On 2015/07/02 08:49:46, peria wrote:
> > > > On 2015/07/02 08:19:36, sof wrote:
> > > > > On 2015/07/02 07:03:27, peria wrote:
> > > > > > On 2015/07/02 06:55:46, peria wrote:
> > > > > > > 
> > > > > > > IMHO, this CL should not add m_fetch.clear() in detach(), and we
> need
> > to
> > > > fix
> > > > > > > the leaks of Document/DocumentLoader separately.
> > > > > > > 
> > > > > > > The reason is as following
> > > > > > > (1) Document and DocumentLoader have already been leaked,
> independent
> > > from
> > > > > > this
> > > > > > > CL,
> > > > > > > (2) Clearing pointers of ResourceFetcher is just a workaround and
it
> > is
> > > > not
> > > > > > > safe.
> > > > > > 
> > > > > > I'm sorry to change directions for many times, and took very long
time
> > to
> > > > work
> > > > > > for them.
> > > > > > I would like to go as I said above.
> > > > > > Sigbjorn, what do you think?
> > > > > 
> > > > > It is temptingly to take the other route, but I think that addressing
> the
> > > test
> > > > > leaks is the workable way to go. Two reasons:
> > > > > 
> > > > >  - ResourceFetcher doesn't have a well-defined lifetime wrt Document.
> > > > >  - When/if other Persistent<>s are temporarily added to Document later
> on,
> > > > we'd
> > > > > have to address the clearing of those too in a timely fashion.
> > > > 
> > > > Let me confirm.  You mean
> > > > 1) we should add m_fetch.clear() in detach()
> > > > 2) test failures should be fixed in reasonable ways
> > > > Is my understanding correct?
> > > 
> > > No, address content_browsertests _leaks_ reported by lsan not _failures_
> from
> > > doing this clearing. i.e., have
> > > https://codereview.chromium.org/1124153003/#msg24 pass.
> > 
> > Simply, you meant "do the Oilpan migration, and fix the leaks", right?
> 
> Yes.

Thank you for the confirmation.
So I decided to
1) migrate classes under core/fetch to Oilpan heap,
2) fix test failures introduced by (1), and
3) suppress the leak report in content_browsertests
in this CL.
And then, I will
4) fix the leak
5) drop the suppression for the leak
in another CL.

It may be better to do (3) before land this CL.

[haraken, 2015-07-03 03:33:20]

On 2015/07/03 02:37:39, peria wrote:
> On 2015/07/02 09:29:28, sof(OOO) wrote:
> > On 2015/07/02 09:22:40, peria wrote:
> > > On 2015/07/02 09:00:38, sof wrote:
> > > > On 2015/07/02 08:49:46, peria wrote:
> > > > > On 2015/07/02 08:19:36, sof wrote:
> > > > > > On 2015/07/02 07:03:27, peria wrote:
> > > > > > > On 2015/07/02 06:55:46, peria wrote:
> > > > > > > > 
> > > > > > > > IMHO, this CL should not add m_fetch.clear() in detach(), and we
> > need
> > > to
> > > > > fix
> > > > > > > > the leaks of Document/DocumentLoader separately.
> > > > > > > > 
> > > > > > > > The reason is as following
> > > > > > > > (1) Document and DocumentLoader have already been leaked,
> > independent
> > > > from
> > > > > > > this
> > > > > > > > CL,
> > > > > > > > (2) Clearing pointers of ResourceFetcher is just a workaround
and
> it
> > > is
> > > > > not
> > > > > > > > safe.
> > > > > > > 
> > > > > > > I'm sorry to change directions for many times, and took very long
> time
> > > to
> > > > > work
> > > > > > > for them.
> > > > > > > I would like to go as I said above.
> > > > > > > Sigbjorn, what do you think?
> > > > > > 
> > > > > > It is temptingly to take the other route, but I think that
addressing
> > the
> > > > test
> > > > > > leaks is the workable way to go. Two reasons:
> > > > > > 
> > > > > >  - ResourceFetcher doesn't have a well-defined lifetime wrt
Document.
> > > > > >  - When/if other Persistent<>s are temporarily added to Document
later
> > on,
> > > > > we'd
> > > > > > have to address the clearing of those too in a timely fashion.
> > > > > 
> > > > > Let me confirm.  You mean
> > > > > 1) we should add m_fetch.clear() in detach()
> > > > > 2) test failures should be fixed in reasonable ways
> > > > > Is my understanding correct?
> > > > 
> > > > No, address content_browsertests _leaks_ reported by lsan not _failures_
> > from
> > > > doing this clearing. i.e., have
> > > > https://codereview.chromium.org/1124153003/#msg24 pass.
> > > 
> > > Simply, you meant "do the Oilpan migration, and fix the leaks", right?
> > 
> > Yes.
> 
> Thank you for the confirmation.
> So I decided to
> 1) migrate classes under core/fetch to Oilpan heap,
> 2) fix test failures introduced by (1), and
> 3) suppress the leak report in content_browsertests
> in this CL.
> And then, I will
> 4) fix the leak
> 5) drop the suppression for the leak
> in another CL.
> 
> It may be better to do (3) before land this CL.

Sounds like a plan.

I noticed that the leak of the second Document doesn't happen in oilpan builds.
This implies that the leak would be fixed once we ship oilpan for the Node
hierarchy.

[peria, 2015-07-03 04:11:43]

On 2015/07/03 03:33:20, haraken wrote:
> On 2015/07/03 02:37:39, peria wrote:
> > On 2015/07/02 09:29:28, sof(OOO) wrote:
> > > On 2015/07/02 09:22:40, peria wrote:
> > > > On 2015/07/02 09:00:38, sof wrote:
> > > > > On 2015/07/02 08:49:46, peria wrote:
> > > > > > On 2015/07/02 08:19:36, sof wrote:
> > > > > > > On 2015/07/02 07:03:27, peria wrote:
> > > > > > > > On 2015/07/02 06:55:46, peria wrote:
> > > > > > > > > 
> > > > > > > > > IMHO, this CL should not add m_fetch.clear() in detach(), and
we
> > > need
> > > > to
> > > > > > fix
> > > > > > > > > the leaks of Document/DocumentLoader separately.
> > > > > > > > > 
> > > > > > > > > The reason is as following
> > > > > > > > > (1) Document and DocumentLoader have already been leaked,
> > > independent
> > > > > from
> > > > > > > > this
> > > > > > > > > CL,
> > > > > > > > > (2) Clearing pointers of ResourceFetcher is just a workaround
> and
> > it
> > > > is
> > > > > > not
> > > > > > > > > safe.
> > > > > > > > 
> > > > > > > > I'm sorry to change directions for many times, and took very
long
> > time
> > > > to
> > > > > > work
> > > > > > > > for them.
> > > > > > > > I would like to go as I said above.
> > > > > > > > Sigbjorn, what do you think?
> > > > > > > 
> > > > > > > It is temptingly to take the other route, but I think that
> addressing
> > > the
> > > > > test
> > > > > > > leaks is the workable way to go. Two reasons:
> > > > > > > 
> > > > > > >  - ResourceFetcher doesn't have a well-defined lifetime wrt
> Document.
> > > > > > >  - When/if other Persistent<>s are temporarily added to Document
> later
> > > on,
> > > > > > we'd
> > > > > > > have to address the clearing of those too in a timely fashion.
> > > > > > 
> > > > > > Let me confirm.  You mean
> > > > > > 1) we should add m_fetch.clear() in detach()
> > > > > > 2) test failures should be fixed in reasonable ways
> > > > > > Is my understanding correct?
> > > > > 
> > > > > No, address content_browsertests _leaks_ reported by lsan not
_failures_
> > > from
> > > > > doing this clearing. i.e., have
> > > > > https://codereview.chromium.org/1124153003/#msg24 pass.
> > > > 
> > > > Simply, you meant "do the Oilpan migration, and fix the leaks", right?
> > > 
> > > Yes.
> > 
> > Thank you for the confirmation.
> > So I decided to
> > 1) migrate classes under core/fetch to Oilpan heap,
> > 2) fix test failures introduced by (1), and
> > 3) suppress the leak report in content_browsertests
> > in this CL.
> > And then, I will
> > 4) fix the leak
> > 5) drop the suppression for the leak
> > in another CL.
> > 
> > It may be better to do (3) before land this CL.
> 
> Sounds like a plan.
> 
> I noticed that the leak of the second Document doesn't happen in oilpan
builds.
> This implies that the leak would be fixed once we ship oilpan for the Node
> hierarchy.

Not right.
This leak also happens on Oilpan builds.

Here is an example of a test on Oilpan.
I added code to print "Document" in ctor and "~Document" in dtor.
This test was run with LSAN, but the leak was not reported.

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from RenderViewImplTest
[ RUN      ] RenderViewImplTest.SetEditableSelectionAndComposition
[30117:30117:0703/130923:682050589657:WARNING:resource_bundle.cc(279)]
locale_file_path.empty() for locale 
Document(0x2b0ebcec1830)
Document(0x2b0ebcec25b8)
~Document(0x2b0ebcec1830)
[       OK ] RenderViewImplTest.SetEditableSelectionAndComposition (332 ms)
[----------] 1 test from RenderViewImplTest (332 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (333 ms total)
[  PASSED  ] 1 test.
LEAK: 8 WebCoreNode
[22/35] RenderViewImplTest.SetEditableSelectionAndComposition (390 ms)

in this case, second Document (0x2b0ebcec25b8) is not destructed.

[haraken, 2015-07-03 04:15:52]

On 2015/07/03 04:11:43, peria wrote:
> On 2015/07/03 03:33:20, haraken wrote:
> > On 2015/07/03 02:37:39, peria wrote:
> > > On 2015/07/02 09:29:28, sof(OOO) wrote:
> > > > On 2015/07/02 09:22:40, peria wrote:
> > > > > On 2015/07/02 09:00:38, sof wrote:
> > > > > > On 2015/07/02 08:49:46, peria wrote:
> > > > > > > On 2015/07/02 08:19:36, sof wrote:
> > > > > > > > On 2015/07/02 07:03:27, peria wrote:
> > > > > > > > > On 2015/07/02 06:55:46, peria wrote:
> > > > > > > > > > 
> > > > > > > > > > IMHO, this CL should not add m_fetch.clear() in detach(),
and
> we
> > > > need
> > > > > to
> > > > > > > fix
> > > > > > > > > > the leaks of Document/DocumentLoader separately.
> > > > > > > > > > 
> > > > > > > > > > The reason is as following
> > > > > > > > > > (1) Document and DocumentLoader have already been leaked,
> > > > independent
> > > > > > from
> > > > > > > > > this
> > > > > > > > > > CL,
> > > > > > > > > > (2) Clearing pointers of ResourceFetcher is just a
workaround
> > and
> > > it
> > > > > is
> > > > > > > not
> > > > > > > > > > safe.
> > > > > > > > > 
> > > > > > > > > I'm sorry to change directions for many times, and took very
> long
> > > time
> > > > > to
> > > > > > > work
> > > > > > > > > for them.
> > > > > > > > > I would like to go as I said above.
> > > > > > > > > Sigbjorn, what do you think?
> > > > > > > > 
> > > > > > > > It is temptingly to take the other route, but I think that
> > addressing
> > > > the
> > > > > > test
> > > > > > > > leaks is the workable way to go. Two reasons:
> > > > > > > > 
> > > > > > > >  - ResourceFetcher doesn't have a well-defined lifetime wrt
> > Document.
> > > > > > > >  - When/if other Persistent<>s are temporarily added to Document
> > later
> > > > on,
> > > > > > > we'd
> > > > > > > > have to address the clearing of those too in a timely fashion.
> > > > > > > 
> > > > > > > Let me confirm.  You mean
> > > > > > > 1) we should add m_fetch.clear() in detach()
> > > > > > > 2) test failures should be fixed in reasonable ways
> > > > > > > Is my understanding correct?
> > > > > > 
> > > > > > No, address content_browsertests _leaks_ reported by lsan not
> _failures_
> > > > from
> > > > > > doing this clearing. i.e., have
> > > > > > https://codereview.chromium.org/1124153003/#msg24 pass.
> > > > > 
> > > > > Simply, you meant "do the Oilpan migration, and fix the leaks", right?
> > > > 
> > > > Yes.
> > > 
> > > Thank you for the confirmation.
> > > So I decided to
> > > 1) migrate classes under core/fetch to Oilpan heap,
> > > 2) fix test failures introduced by (1), and
> > > 3) suppress the leak report in content_browsertests
> > > in this CL.
> > > And then, I will
> > > 4) fix the leak
> > > 5) drop the suppression for the leak
> > > in another CL.
> > > 
> > > It may be better to do (3) before land this CL.
> > 
> > Sounds like a plan.
> > 
> > I noticed that the leak of the second Document doesn't happen in oilpan
> builds.
> > This implies that the leak would be fixed once we ship oilpan for the Node
> > hierarchy.
> 
> Not right.
> This leak also happens on Oilpan builds.
> 
> Here is an example of a test on Oilpan.
> I added code to print "Document" in ctor and "~Document" in dtor.
> This test was run with LSAN, but the leak was not reported.
> 
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from RenderViewImplTest
> [ RUN      ] RenderViewImplTest.SetEditableSelectionAndComposition
> [30117:30117:0703/130923:682050589657:WARNING:resource_bundle.cc(279)]
> locale_file_path.empty() for locale 
> Document(0x2b0ebcec1830)
> Document(0x2b0ebcec25b8)
> ~Document(0x2b0ebcec1830)
> [       OK ] RenderViewImplTest.SetEditableSelectionAndComposition (332 ms)
> [----------] 1 test from RenderViewImplTest (332 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (333 ms total)
> [  PASSED  ] 1 test.
> LEAK: 8 WebCoreNode
> [22/35] RenderViewImplTest.SetEditableSelectionAndComposition (390 ms)
> 
> in this case, second Document (0x2b0ebcec25b8) is not destructed.

oh, on oilpan build, the second Document doesn't leak in
DidFailProvisionalLoadWithErrorForCancellation but it leaks in another test...

[haraken, 2015-07-03 06:12:27]

On 2015/07/03 04:15:52, haraken wrote:
> On 2015/07/03 04:11:43, peria wrote:
> > On 2015/07/03 03:33:20, haraken wrote:
> > > On 2015/07/03 02:37:39, peria wrote:
> > > > On 2015/07/02 09:29:28, sof(OOO) wrote:
> > > > > On 2015/07/02 09:22:40, peria wrote:
> > > > > > On 2015/07/02 09:00:38, sof wrote:
> > > > > > > On 2015/07/02 08:49:46, peria wrote:
> > > > > > > > On 2015/07/02 08:19:36, sof wrote:
> > > > > > > > > On 2015/07/02 07:03:27, peria wrote:
> > > > > > > > > > On 2015/07/02 06:55:46, peria wrote:
> > > > > > > > > > > 
> > > > > > > > > > > IMHO, this CL should not add m_fetch.clear() in detach(),
> and
> > we
> > > > > need
> > > > > > to
> > > > > > > > fix
> > > > > > > > > > > the leaks of Document/DocumentLoader separately.
> > > > > > > > > > > 
> > > > > > > > > > > The reason is as following
> > > > > > > > > > > (1) Document and DocumentLoader have already been leaked,
> > > > > independent
> > > > > > > from
> > > > > > > > > > this
> > > > > > > > > > > CL,
> > > > > > > > > > > (2) Clearing pointers of ResourceFetcher is just a
> workaround
> > > and
> > > > it
> > > > > > is
> > > > > > > > not
> > > > > > > > > > > safe.
> > > > > > > > > > 
> > > > > > > > > > I'm sorry to change directions for many times, and took very
> > long
> > > > time
> > > > > > to
> > > > > > > > work
> > > > > > > > > > for them.
> > > > > > > > > > I would like to go as I said above.
> > > > > > > > > > Sigbjorn, what do you think?
> > > > > > > > > 
> > > > > > > > > It is temptingly to take the other route, but I think that
> > > addressing
> > > > > the
> > > > > > > test
> > > > > > > > > leaks is the workable way to go. Two reasons:
> > > > > > > > > 
> > > > > > > > >  - ResourceFetcher doesn't have a well-defined lifetime wrt
> > > Document.
> > > > > > > > >  - When/if other Persistent<>s are temporarily added to
Document
> > > later
> > > > > on,
> > > > > > > > we'd
> > > > > > > > > have to address the clearing of those too in a timely fashion.
> > > > > > > > 
> > > > > > > > Let me confirm.  You mean
> > > > > > > > 1) we should add m_fetch.clear() in detach()
> > > > > > > > 2) test failures should be fixed in reasonable ways
> > > > > > > > Is my understanding correct?
> > > > > > > 
> > > > > > > No, address content_browsertests _leaks_ reported by lsan not
> > _failures_
> > > > > from
> > > > > > > doing this clearing. i.e., have
> > > > > > > https://codereview.chromium.org/1124153003/#msg24 pass.
> > > > > > 
> > > > > > Simply, you meant "do the Oilpan migration, and fix the leaks",
right?
> > > > > 
> > > > > Yes.
> > > > 
> > > > Thank you for the confirmation.
> > > > So I decided to
> > > > 1) migrate classes under core/fetch to Oilpan heap,
> > > > 2) fix test failures introduced by (1), and
> > > > 3) suppress the leak report in content_browsertests
> > > > in this CL.
> > > > And then, I will
> > > > 4) fix the leak
> > > > 5) drop the suppression for the leak
> > > > in another CL.
> > > > 
> > > > It may be better to do (3) before land this CL.
> > > 
> > > Sounds like a plan.
> > > 
> > > I noticed that the leak of the second Document doesn't happen in oilpan
> > builds.
> > > This implies that the leak would be fixed once we ship oilpan for the Node
> > > hierarchy.
> > 
> > Not right.
> > This leak also happens on Oilpan builds.
> > 
> > Here is an example of a test on Oilpan.
> > I added code to print "Document" in ctor and "~Document" in dtor.
> > This test was run with LSAN, but the leak was not reported.
> > 
> > [==========] Running 1 test from 1 test case.
> > [----------] Global test environment set-up.
> > [----------] 1 test from RenderViewImplTest
> > [ RUN      ] RenderViewImplTest.SetEditableSelectionAndComposition
> > [30117:30117:0703/130923:682050589657:WARNING:resource_bundle.cc(279)]
> > locale_file_path.empty() for locale 
> > Document(0x2b0ebcec1830)
> > Document(0x2b0ebcec25b8)
> > ~Document(0x2b0ebcec1830)
> > [       OK ] RenderViewImplTest.SetEditableSelectionAndComposition (332 ms)
> > [----------] 1 test from RenderViewImplTest (332 ms total)
> > 
> > [----------] Global test environment tear-down
> > [==========] 1 test from 1 test case ran. (333 ms total)
> > [  PASSED  ] 1 test.
> > LEAK: 8 WebCoreNode
> > [22/35] RenderViewImplTest.SetEditableSelectionAndComposition (390 ms)
> > 
> > in this case, second Document (0x2b0ebcec25b8) is not destructed.
> 
> oh, on oilpan build, the second Document doesn't leak in
> DidFailProvisionalLoadWithErrorForCancellation but it leaks in another test...

Hmm, I tried to call GetMainFrame()->close, WebHeap::collectAllGarbage in
RenderViewTest::TearDown in various ways, but I've not succeeded in fixing the
leaks...

[peria, 2015-07-06 05:51:23]

Patchset #23 (id:680001) has been deleted

[peria, 2015-07-07 09:48:27]

Hi, I'm glad to announce that this CL is ready to be reviewed again.
It no longer contains strange hacks to pass tests.

[haraken, 2015-07-07 13:02:43]

LGTM

https://codereview.chromium.org/1124153003/diff/700001/Source/core/loader/Fra...
File Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/1124153003/diff/700001/Source/core/loader/Fra...
Source/core/loader/FrameFetchContextTest.cpp:119: Persistent<FrameFetchContext>
fetchContext;

Can we use Persistent<FetchContext>? Then you won't need the above static_cast
and the below static_cast.

[peria, 2015-07-08 02:36:09]

https://codereview.chromium.org/1124153003/diff/700001/Source/core/loader/Fra...
File Source/core/loader/FrameFetchContextTest.cpp (right):

https://codereview.chromium.org/1124153003/diff/700001/Source/core/loader/Fra...
Source/core/loader/FrameFetchContextTest.cpp:119: Persistent<FrameFetchContext>
fetchContext;
On 2015/07/07 13:02:42, haraken wrote:
> 
> Can we use Persistent<FetchContext>? Then you won't need the above static_cast
> and the below static_cast.

Done.

[peria, 2015-07-08 03:16:06]

The CQ bit was checked by peria@chromium.org

[peria, 2015-07-08 03:16:07]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1124153003/#ps720001
(title: "Work for comment")

[commit-bot: I haz the power, 2015-07-08 03:16:20]

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

[commit-bot: I haz the power, 2015-07-08 04:47:49]

Committed patchset #24 (id:720001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198461

[Nico, 2015-08-25 04:22:59]

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

[Nico, 2015-08-25 04:23:01]

https://codereview.chromium.org/1124153003/diff/720001/Source/core/fetch/Reso...
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1124153003/diff/720001/Source/core/fetch/Reso...
Source/core/fetch/Resource.h:343: PersistentWillBeMember<ResourceLoader>
m_loader;
Some tests leak m_loader, for example:
http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxASan%20tester/...

Can you take a look? Do tests force a gc before the test completes?

[peria, 2015-08-25 07:48:14]

https://codereview.chromium.org/1124153003/diff/720001/Source/core/fetch/Reso...
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1124153003/diff/720001/Source/core/fetch/Reso...
Source/core/fetch/Resource.h:343: PersistentWillBeMember<ResourceLoader>
m_loader;
On 2015/08/25 04:23:01, Nico wrote:
> Some tests leak m_loader, for example:
>
http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxASan%20tester/...
> 
> Can you take a look? Do tests force a gc before the test completes?

I confirmed this Persistent leaked a loader.
I'll take a look.

[Nico, 2015-08-25 15:28:32]

On 2015/08/25 07:48:14, peria wrote:
>
https://codereview.chromium.org/1124153003/diff/720001/Source/core/fetch/Reso...
> File Source/core/fetch/Resource.h (right):
> 
>
https://codereview.chromium.org/1124153003/diff/720001/Source/core/fetch/Reso...
> Source/core/fetch/Resource.h:343: PersistentWillBeMember<ResourceLoader>
> m_loader;
> On 2015/08/25 04:23:01, Nico wrote:
> > Some tests leak m_loader, for example:
> >
>
http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxASan%20tester/...
> > 
> > Can you take a look? Do tests force a gc before the test completes?
> 
> I confirmed this Persistent leaked a loader.
> I'll take a look.

Any luck?

[peria, 2015-08-26 03:10:56]

On 2015/08/25 15:28:32, Nico wrote:
> Any luck?

I checked how it had behaved before landing this patch,
and I find that this leak has existed before this CL.
It was not detected because some tests in ResourceFetcherTest were
added after this CL.

IIUC, ResourceFetcher, ResourceLoader, and Resource have a reference cycle
and it is the cause of this leak.

before this CL:
 ResourceFetcher =(OwnPtr)=> ResourceLoaderSet =(RefPtr)=> ResourceLoader
-(RawPtr)-> Resource
 ResourceFetcher <=(RefPtr)=                               ResourceLoader
<=(RefPtr)= Resource
 ResourceFetcher =(HashMap)=> ResourcePtr -(RawPtr)->                           
     Resource

after this CL (w/o Oilpan):
 ResourceFetcher =(Member)=> ResourceLoaderSet =(Member)=> ResourceLoader
-(RawPtr)-> Resource
 ResourceFetcher <=(Member)=                               ResourceLoader
<=(Persistent)= Resource
 ResourceFetcher =(HeapHashMap)=> ResourcePtr -(RawPtr)->                       
     Resource

[Nico, 2015-08-26 03:16:39]

How can we fix this? Who knows how this is supposed to work? japhet? abarth?
On Aug 25, 2015 8:10 PM, <peria@chromium.org> wrote:

> On 2015/08/25 15:28:32, Nico wrote:
>
>> Any luck?
>>
>
> I checked how it had behaved before landing this patch,
> and I find that this leak has existed before this CL.
> It was not detected because some tests in ResourceFetcherTest were
> added after this CL.
>
> IIUC, ResourceFetcher, ResourceLoader, and Resource have a reference cycle
> and it is the cause of this leak.
>
> before this CL:
>  ResourceFetcher =(OwnPtr)=> ResourceLoaderSet =(RefPtr)=> ResourceLoader
> -(RawPtr)-> Resource
>  ResourceFetcher <=(RefPtr)=                               ResourceLoader
> <=(RefPtr)= Resource
>  ResourceFetcher =(HashMap)=> ResourcePtr -(RawPtr)->
>      Resource
>
> after this CL (w/o Oilpan):
>  ResourceFetcher =(Member)=> ResourceLoaderSet =(Member)=> ResourceLoader
> -(RawPtr)-> Resource
>  ResourceFetcher <=(Member)=                               ResourceLoader
> <=(Persistent)= Resource
>  ResourceFetcher =(HeapHashMap)=> ResourcePtr -(RawPtr)->
>      Resource
>
>
> https://codereview.chromium.org/1124153003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[peria, 2015-08-26 03:27:50]

On 2015/08/26 03:16:39, Nico wrote:
> How can we fix this? Who knows how this is supposed to work? japhet? abarth?

I think japhet@ knows well.
He added those tests and seems to work actively on core/fetch/.

[Nico, 2015-08-29 21:52:53]

I filed http://crbug.com/526423 for the leaks. It's almost the last leak
remaining in blink tests by now.

On Tue, Aug 25, 2015 at 8:27 PM, <peria@chromium.org> wrote:

> On 2015/08/26 03:16:39, Nico wrote:
>
>> How can we fix this? Who knows how this is supposed to work? japhet?
>> abarth?
>>
>
> I think japhet@ knows well.
> He added those tests and seems to work actively on core/fetch/.
>
> https://codereview.chromium.org/1124153003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.