Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18375005:
[oilpan] Move CSSFontFace and CSSSegmentedFontFace to the managed heap (Closed)
|
Created:
7 years, 5 months ago by haraken Modified:
7 years, 5 months ago Reviewers:
zerny-google, Vyacheslav Egorov (Chromium), Erik Corry, Mads Ager (google), Mads Ager (chromium) CC:
blink-reviews, adamk+oilpan_chromium.org, abarth-chromium Visibility:
Public. |
Description[oilpan] Move CSSFontFace and CSSSegmentedFontFace to the managed heap
Split into CLs:
https://codereview.chromium.org/18856015/
https://codereview.chromium.org/18882002/
Patch Set 1 #
Total comments: 12
Patch Set 2 : #
Total comments: 9
Patch Set 3 : #Patch Set 4 : #
Total comments: 3
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Total comments: 1
Patch Set 9 : #
Total comments: 1
Patch Set 10 : #
Messages
Total messages: 16 (0 generated)
PTAL
Hi Haraken, A few comments in passing. https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSFontFace.h File Source/core/css/CSSFontFace.h (right): https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSFontFace.h... Source/core/css/CSSFontFace.h:36: #include <wtf/RefCounted.h> Remove. https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSFontFace.h... Source/core/css/CSSFontFace.h:94: void accept(Visitor*) const { } visit m_segmentedFontFaces and m_rule. https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSFontSelect... File Source/core/css/CSSFontSelector.cpp (right): https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSFontSelect... Source/core/css/CSSFontSelector.cpp:306: for (unsigned i = 0; i < numLocallyInstalledFaces; ++i) { Handle scope. https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSFontSelect... Source/core/css/CSSFontSelector.cpp:536: for (unsigned i = 0; i < numLocallyInstalledFontFaces; ++i) { Handle scope https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSSegmentedF... File Source/core/css/CSSSegmentedFontFace.cpp (right): https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSSegmentedF... Source/core/css/CSSSegmentedFontFace.cpp:189: visitor->visit(m_fontFaces); visit m_callbacks https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSSegmentedF... File Source/core/css/CSSSegmentedFontFace.h (right): https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSSegmentedF... Source/core/css/CSSSegmentedFontFace.h:32: #include <wtf/RefCounted.h> Remove
Thanks for reviewing, updated the CL. https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSFontFace.h File Source/core/css/CSSFontFace.h (right): https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSFontFace.h... Source/core/css/CSSFontFace.h:36: #include <wtf/RefCounted.h> On 2013/07/02 14:24:04, zerny wrote: > Remove. Done. https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSFontFace.h... Source/core/css/CSSFontFace.h:94: void accept(Visitor*) const { } This is a big mistake... Done. https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSFontSelect... File Source/core/css/CSSFontSelector.cpp (right): https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSFontSelect... Source/core/css/CSSFontSelector.cpp:306: for (unsigned i = 0; i < numLocallyInstalledFaces; ++i) { On 2013/07/02 14:24:04, zerny wrote: > Handle scope. Done. https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSFontSelect... Source/core/css/CSSFontSelector.cpp:536: for (unsigned i = 0; i < numLocallyInstalledFontFaces; ++i) { On 2013/07/02 14:24:04, zerny wrote: > Handle scope Done. https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSSegmentedF... File Source/core/css/CSSSegmentedFontFace.cpp (right): https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSSegmentedF... Source/core/css/CSSSegmentedFontFace.cpp:189: visitor->visit(m_fontFaces); On 2013/07/02 14:24:04, zerny wrote: > visit m_callbacks Done. https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSSegmentedF... File Source/core/css/CSSSegmentedFontFace.h (right): https://codereview.chromium.org/18375005/diff/1/Source/core/css/CSSSegmentedF... Source/core/css/CSSSegmentedFontFace.h:32: #include <wtf/RefCounted.h> On 2013/07/02 14:24:04, zerny wrote: > Remove Done.
https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontFac... File Source/core/css/CSSFontFace.cpp (right): https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontFac... Source/core/css/CSSFontFace.cpp:171: for (HashSet<Member<CSSSegmentedFontFace> >::const_iterator it = m_segmentedFontFaces.begin(); it != m_segmentedFontFaces.end(); ++it) Let's add this one in visitor next to visit(Vector<Member<T> >) so that we do not have to repeat this loop for other hash sets and can just write visitor->visit(m_segmentedFontFaces) here. https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontFac... File Source/core/css/CSSFontFaceSource.h (right): https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontFac... Source/core/css/CSSFontFaceSource.h:81: Persistent<CSSFontFace> m_face; // Our owning font face. This looks like a definite leak. The CSSFontFace has the source in an OwnPtr. But now, the CSSFontFace cannot die because the CSSFontFace holds a persistent to it. Either leave this as a raw pointer with a FIXME(oilpan) comment or add a FIXME(oilpan) comment stating that this is a leak and work on moving the CSSFontFaceSource to the oilpan heap. https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel... File Source/core/css/CSSFontSelector.cpp (right): https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel... Source/core/css/CSSFontSelector.cpp:512: Persistent<CSSSegmentedFontFace>& face = segmentedFontFaceCache->add(traitsMask, nullptr).iterator->value; I don't think we should be using this pattern. The problem is that the create call below can allocate. That allocation can cause a GC which can call finalizers. If one of those finalizers changes the segmentedFontFaceCache we can get a memory corruption here. Instead, we should do something like: Handle<CSSSegmentedFontFace> face = segmentedFontFaceCache->add(traitsMask, nullptr).iterator->value; if (value) return value; face = CSSSegmentedFontFace::create(this); segmentedFontFaceCache->set(traitsMask, face); ...
https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontFac... File Source/core/css/CSSFontFace.cpp (right): https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontFac... Source/core/css/CSSFontFace.cpp:171: for (HashSet<Member<CSSSegmentedFontFace> >::const_iterator it = m_segmentedFontFaces.begin(); it != m_segmentedFontFaces.end(); ++it) On 2013/07/02 14:59:23, Mads Ager (chromium) wrote: > Let's add this one in visitor next to visit(Vector<Member<T> >) so that we do > not have to repeat this loop for other hash sets and can just write > visitor->visit(m_segmentedFontFaces) here. Done. https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontFac... File Source/core/css/CSSFontFaceSource.h (right): https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontFac... Source/core/css/CSSFontFaceSource.h:81: Persistent<CSSFontFace> m_face; // Our owning font face. Good point. Done. https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel... File Source/core/css/CSSFontSelector.cpp (right): https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel... Source/core/css/CSSFontSelector.cpp:295: OwnPtr<CSSFontFaceVectorCollection>& familyFontFaces = m_fontFaces.add(familyName, nullptr).iterator->value; What do you think about this? We cannot simply change this to: OwnPtr<CSSFontFaceVectorCollection> familyFontFaces = ... because OwnPtr is not copyable. One way to solve this issue is to store CSSFontFaceVectorCollection instead of OwnPtr<CSSFontFaceVectorCollection>. However, in that case, we have to copy CollectionRoot a lot of times. https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel... Source/core/css/CSSFontSelector.cpp:512: Persistent<CSSSegmentedFontFace>& face = segmentedFontFaceCache->add(traitsMask, nullptr).iterator->value; Done.
https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel... File Source/core/css/CSSFontSelector.cpp (right): https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel... Source/core/css/CSSFontSelector.cpp:295: OwnPtr<CSSFontFaceVectorCollection>& familyFontFaces = m_fontFaces.add(familyName, nullptr).iterator->value; On 2013/07/03 04:39:20, haraken wrote: > > What do you think about this? We cannot simply change this to: > > OwnPtr<CSSFontFaceVectorCollection> familyFontFaces = ... > > because OwnPtr is not copyable. > > One way to solve this issue is to store CSSFontFaceVectorCollection instead of > OwnPtr<CSSFontFaceVectorCollection>. However, in that case, we have to copy > CollectionRoot a lot of times. Yeah, this one is a little annoying. Another way would be to pull out the actual vector pointer and operate on that directly: CSSFontFaceVectorCollection* familyFontFaces = m_fontFaces.add(familyName, nullptr).iterator->value.get(); if (!familyFontFaces) { familyFontFaces = new CSSFontFaceVectorCollection; m_fontFaces.set(familyName, adoptPtr(familyFontFaces)); ... } ... We should consider how to deal with these heap allocated vectors. If we start allocating them on the oilpan heap we could replace the OwnPtr with Member and extract them in a Handle. https://codereview.chromium.org/18375005/diff/17001/Source/core/css/CSSFontSe... File Source/core/css/CSSFontSelector.cpp (right): https://codereview.chromium.org/18375005/diff/17001/Source/core/css/CSSFontSe... Source/core/css/CSSFontSelector.cpp:506: OwnPtr<HashMap<unsigned, Persistent<CSSSegmentedFontFace> > >& segmentedFontFaceCache = m_fonts.add(family, nullptr).iterator->value; And here is another one. https://codereview.chromium.org/18375005/diff/17001/Source/core/css/CSSFontSe... Source/core/css/CSSFontSelector.cpp:512: Persistent<CSSSegmentedFontFace> face = segmentedFontFaceCache->add(traitsMask, nullptr).iterator->value; This should just be Handle instead of Persistent.
Comments addressed. Updated the CL. PTAL. https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel... File Source/core/css/CSSFontSelector.cpp (right): https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel... Source/core/css/CSSFontSelector.cpp:295: OwnPtr<CSSFontFaceVectorCollection>& familyFontFaces = m_fontFaces.add(familyName, nullptr).iterator->value; On 2013/07/03 06:30:42, Mads Ager (chromium) wrote: > On 2013/07/03 04:39:20, haraken wrote: > > > > What do you think about this? We cannot simply change this to: > > > > OwnPtr<CSSFontFaceVectorCollection> familyFontFaces = ... > > > > because OwnPtr is not copyable. > > > > One way to solve this issue is to store CSSFontFaceVectorCollection instead of > > OwnPtr<CSSFontFaceVectorCollection>. However, in that case, we have to copy > > CollectionRoot a lot of times. > > Yeah, this one is a little annoying. Another way would be to pull out the actual > vector pointer and operate on that directly: > > CSSFontFaceVectorCollection* familyFontFaces = m_fontFaces.add(familyName, > nullptr).iterator->value.get(); > if (!familyFontFaces) { > familyFontFaces = new CSSFontFaceVectorCollection; > m_fontFaces.set(familyName, adoptPtr(familyFontFaces)); > ... > } > ... > > We should consider how to deal with these heap allocated vectors. If we start > allocating them on the oilpan heap we could replace the OwnPtr with Member and > extract them in a Handle. Done. https://codereview.chromium.org/18375005/diff/17001/Source/core/css/CSSFontSe... File Source/core/css/CSSFontSelector.cpp (right): https://codereview.chromium.org/18375005/diff/17001/Source/core/css/CSSFontSe... Source/core/css/CSSFontSelector.cpp:512: Persistent<CSSSegmentedFontFace> face = segmentedFontFaceCache->add(traitsMask, nullptr).iterator->value; On 2013/07/03 06:30:42, Mads Ager (chromium) wrote: > This should just be Handle instead of Persistent. Done.
LGTM!
I made two changes to pass all tests. Landing... https://codereview.chromium.org/18375005/diff/22001/Source/core/css/CSSFontSe... File Source/core/css/CSSFontSelector.cpp (right): https://codereview.chromium.org/18375005/diff/22001/Source/core/css/CSSFontSe... Source/core/css/CSSFontSelector.cpp:531: Vector<Handle<CSSFontFace>, 32> candidateFontFaces; This is wrong. Handles in the vector are freed in the HandleScope in the below for-loop. This should be CollectionRoot<Vector<Member<CSSFonFace>>>. (I hope this kind of errors are auto-detected by a verification framework.) https://codereview.chromium.org/18375005/diff/28001/Source/core/css/CSSSegmen... File Source/core/css/CSSSegmentedFontFace.cpp (left): https://codereview.chromium.org/18375005/diff/28001/Source/core/css/CSSSegmen... Source/core/css/CSSSegmentedFontFace.cpp:50: m_fontFaces[i]->removedFromSegmentedFontFace(this); We have to remove this. Since we're in a destructor, there is a possibility that Members in the HashSet are already freed. (Now I understand erik's point that we don't want to call destructors in the sweep phase -- it's likely to cause use-after-free.)
https://codereview.chromium.org/18375005/diff/22001/Source/core/css/CSSFontSe... File Source/core/css/CSSFontSelector.cpp (right): https://codereview.chromium.org/18375005/diff/22001/Source/core/css/CSSFontSe... Source/core/css/CSSFontSelector.cpp:531: Vector<Handle<CSSFontFace>, 32> candidateFontFaces; On 2013/07/03 12:03:23, haraken wrote: > > This is wrong. Handles in the vector are freed in the HandleScope in the below > for-loop. This should be CollectionRoot<Vector<Member<CSSFonFace>>>. > > (I hope this kind of errors are auto-detected by a verification framework.) Yes, good catch. https://codereview.chromium.org/18375005/diff/28001/Source/core/css/CSSSegmen... File Source/core/css/CSSSegmentedFontFace.cpp (left): https://codereview.chromium.org/18375005/diff/28001/Source/core/css/CSSSegmen... Source/core/css/CSSSegmentedFontFace.cpp:50: m_fontFaces[i]->removedFromSegmentedFontFace(this); On 2013/07/03 12:03:23, haraken wrote: > > We have to remove this. Since we're in a destructor, there is a possibility that > Members in the HashSet are already freed. > > (Now I understand erik's point that we don't want to call destructors in the > sweep phase -- it's likely to cause use-after-free.) Right, just to be clear on this. The lifetime of the FontFace and the SegmentedFontFace are now always linked, right? So this code is safe to remove since both things die at the same time?
> Right, just to be clear on this. The lifetime of the FontFace and the > SegmentedFontFace are now always linked, right? So this code is safe to remove > since both things die at the same time? Exactly. Their lifetime is now in sync.
https://codereview.chromium.org/18375005/diff/33001/Source/core/css/CSSSegmen... File Source/core/css/CSSSegmentedFontFace.h (right): https://codereview.chromium.org/18375005/diff/33001/Source/core/css/CSSSegmen... Source/core/css/CSSSegmentedFontFace.h:78: RefPtr<CSSFontSelector> m_fontSelector; Adding this RefPtr fixed the heap corruption in SVG tests. Given that CSSSegmentedFontFace is responsible for keeping the lifetime of CSSFontSelector, I think this should be a RefPtr. My question is why the previous code had been working without the RefPtr... If you think it's OK to land with the RefPtr, I'll land this CL. If you're skeptical about the RefPtr, I'll look into why the previous code had been working without the RefPtr.
On 2013/07/04 08:03:44, haraken wrote: > https://codereview.chromium.org/18375005/diff/33001/Source/core/css/CSSSegmen... > File Source/core/css/CSSSegmentedFontFace.h (right): > > https://codereview.chromium.org/18375005/diff/33001/Source/core/css/CSSSegmen... > Source/core/css/CSSSegmentedFontFace.h:78: RefPtr<CSSFontSelector> > m_fontSelector; > > Adding this RefPtr fixed the heap corruption in SVG tests. Given that > CSSSegmentedFontFace is responsible for keeping the lifetime of CSSFontSelector, > I think this should be a RefPtr. > > My question is why the previous code had been working without the RefPtr... If > you think it's OK to land with the RefPtr, I'll land this CL. If you're > skeptical about the RefPtr, I'll look into why the previous code had been > working without the RefPtr. I think we need to look a bit deeper here. Adding a RefPtr here will probably create a cycle: CSSFontSelector has a mapping containing Persistent<CSSSegmentedFontFace> and therefore if we put a RefPtr<CSSFontSelector> in the segmented font face we have a cycle that we cannot reclaim. I think we need to understand where the corruption comes from. Have you tried running this under ASAN?
> I think we need to look a bit deeper here. Adding a RefPtr here will probably > create a cycle: CSSFontSelector has a mapping containing > Persistent<CSSSegmentedFontFace> and therefore if we put a > RefPtr<CSSFontSelector> in the segmented font face we have a cycle that we > cannot reclaim. I think we need to understand where the corruption comes from. > Have you tried running this under ASAN? ok, will take a look. ASAN didn't complain about anything, probably because ASAN uses a different allocator.
hmm, still looking into the crash cause... With my CL, some unexpected message loop is executed and crashes. $./out/Debug/DumpRenderTree third_party/WebKit/LayoutTests/svg/W3C-SVG-1.1-SE/struct-dom-11-f.svg Received signal 11 SEGV_MAPERR 0000bbadbeef [0x000002de841e] base::debug::StackTrace::StackTrace() [0x000002de7f55] base::debug::(anonymous namespace)::StackDumpSignalHandler() [0x7f5ed3d1ecb0] <unknown> [0x0000025ea55e] WTF::VectorBufferBase<>::allocateBuffer() [0x0000025ea444] WTF::Vector<>::reserveCapacity() [0x0000025ea390] WTF::Vector<>::expandCapacity() [0x0000025e93c5] WTF::Vector<>::resize() [0x0000025e11e3] WTF::copyToVector<>() [0x0000025df143] WebCore::CSSFontSelector::dispatchInvalidationCallbacks() [0x0000025df285] WebCore::CSSFontSelector::fontLoaded() [0x000002738586] WebCore::CSSFontFace::fontLoaded() [0x0000025d8e22] WebCore::CSSFontFaceSource::fontLoaded() [0x000002727d3b] WebCore::CachedFont::checkNotify() [0x00000243de7d] WebCore::CachedResource::error() [0x0000024150ff] WebCore::ResourceLoader::didFail() [0x000002ae5a8a] WebCore::ResourceHandleInternal::didFail() [0x00000286f92f] webkit_glue::WebURLLoaderImpl::Context::OnCompletedRequest() [0x0000035c198a] (anonymous namespace)::RequestProxy::NotifyCompletedRequest() [0x0000035c1dc4] base::internal::RunnableAdapter<>::Run() [0x0000035c1cfb] base::internal::InvokeHelper<>::MakeItSo() [0x0000035c1c8a] base::internal::Invoker<>::Run() [0x00000202002e] base::Callback<>::Run() <---- This Run() is not invoked in the trunk. I minimized my CL to the patch set 9 (which causes the crash). The patch set 9 looks completely correct to me... Looking. https://codereview.chromium.org/18375005/diff/39001/Source/core/css/CSSFontSe... File Source/core/css/CSSFontSelector.cpp (right): https://codereview.chromium.org/18375005/diff/39001/Source/core/css/CSSFontSe... Source/core/css/CSSFontSelector.cpp:519: segmentedFontFaceCache->set(traitsMask, face); The only suspicious place is here, but I confirmed that the call path around here is exactly the same between with and without my CL.
I finally found the cause of the crash. There are two complicated factors about lifetime management of CSSFontFace, which is hard to explain here. In short, [1] CSSFontSelector has a Persistent to CSSSegmentedFontFace. CSSSegmentedFontFace has a back pointer (raw pointer) to CSSFontSelector. Thus, we have to explicitly clear the back pointer when destructing the Persistent. [2] CSSSegmentedFontFace has a Member to CSSFontFace. CSSFontFace also has a back pointer to CSSSegmentedFontFace. Thus, we have to clear the back pointer when destructing the Member. (Note: Even in the world where we move both CSSSegmentedFontFace and CSSFontFace to the managed heap, we cannot express the back pointer as a Member for some complicated reason.) I'll upload a CL to fix these problems soon. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
