Bug 105345 – Replace documentFragmentIsShadowRoot with isTreeScope

• Component: New Bugs
• Status: RESOLVED FIXED

[Elliott Sprehn]

Replace documentFragmentIsShadowRoot with isTreeScope

[Elliott Sprehn]

Created attachment 180022 [details]
Patch

[Elliott Sprehn]

Created attachment 180030 [details]
Patch

[Build Bot]

Comment on attachment 180030 [details]
Patch

Attachment 180030 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15413296

[Elliott Sprehn]

(In reply to comment #3)
> (From update of attachment 180030 [details])
> Attachment 180030 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/15413296

Hmm, gtk says:

ls.a(libWebCoreInternals_la-Internals.o):Internals.cpp:function WebCore::Internals::numberOfScopedHTMLStyleChildren(WebCore::Node const*, int&) const: error: undefined reference to 'WebCore::Node::isTreeScope() const'

and win-ews says:

16>WebCoreTestSupport.lib(Internals.obj) : error LNK2019: unresolved external symbol "public: bool __thiscall WebCore::Node::isTreeScope(void)const " (?isTreeScope@Node@WebCore@@QBE_NXZ) referenced in function "public: bool __thiscall WebCore::Node::isShadowRoot(void)const " (?isShadowRoot@Node@WebCore@@QBE_NXZ)
16>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Debug\bin\DumpRenderTree.dll : fatal error LNK1120: 1 unresolved externals

Looks like we need to export isTreeScope which wasn't needed for documentFragmentIsShadowRoot since it was virtual? I'll upload again with a WebCore.exp.in change.

[Build Bot]

Comment on attachment 180030 [details]
Patch

Attachment 180030 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15399493

[Elliott Sprehn]

Created attachment 180432 [details]
Patch

[Build Bot]

Comment on attachment 180432 [details]
Patch

Attachment 180432 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15460128

[Elliott Sprehn]

Created attachment 180445 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 180445 [details]
Patch

You can also just do webkit-patch land-safely. You're a committer now, right?

[Elliott Sprehn]

(In reply to comment #9)
> (From update of attachment 180445 [details])
> You can also just do webkit-patch land-safely. You're a committer now, right?

Yeah, I realized I had cq+'ed before the gtk build had gone green so I removed it to wait to see if my exports file changes would work. :)

[WebKit Review Bot]

Comment on attachment 180445 [details]
Patch

Clearing flags on attachment: 180445

Committed r138338: <http://trac.webkit.org/changeset/138338>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 105621

[Tim Horton]

This also broke the Apple Windows port build for some unknown reason.

[Ilya Tikhonovsky]

(In reply to comment #14)
> This also broke the Apple Windows port build for some unknown reason.

STDOUT: <empty>
STDERR: ASSERTION FAILED: scope
STDERR: /Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../html/HTMLStyleElement.cpp(160) : void WebCore::HTMLStyleElement::unregisterWithScopingNode(WebCore::ContainerNode *)
STDERR: 1   0x1d74190 WebCore::HTMLStyleElement::unregisterWithScopingNode(WebCore::ContainerNode*)
STDERR: 2   0x1d748d3 WebCore::HTMLStyleElement::removedFrom(WebCore::ContainerNode*)
STDERR: 3   0x180dedf WebCore::ChildNodeRemovalNotifier::notifyNodeRemovedFromDocument(WebCore::Node*)
STDERR: 4   0x18107dc WebCore::ChildNodeRemovalNotifier::notifyDescendantRemovedFromDocument(WebCore::ContainerNode*)
STDERR: 5   0x180df11 WebCore::ChildNodeRemovalNotifier::notifyNodeRemovedFromDocument(WebCore::Node*)
STDERR: 6   0x180a967 WebCore::ChildNodeRemovalNotifier::notify(WebCore::Node*)
STDERR: 7   0x18f2d0d WebCore::ElementShadow::removeAllShadowRoots()
STDERR: 8   0x18f7db4 WebCore::Element::~Element()
STDERR: 9   0x1a2c022 WebCore::StyledElement::~StyledElement()
STDERR: 10  0x1d1f79b WebCore::HTMLElement::~HTMLElement()
STDERR: 11  0x1f02d0b WebCore::HTMLDivElement::~HTMLDivElement()
STDERR: 12  0x1cac25b WebCore::HTMLDivElement::~HTMLDivElement()
STDERR: 13  0x1cac2ae WebCore::HTMLDivElement::~HTMLDivElement()
STDERR: 14  0x180a0dd void WebCore::removeAllChildrenInContainer<WebCore::Node, WebCore::ContainerNode>(WebCore::ContainerNode*)
STDERR: 15  0x180411b WebCore::ContainerNode::removeAllChildren()
STDERR: 16  0x182a26d WebCore::Document::removedLastRef()
STDERR: 17  0x1987ab6 WebCore::Node::removedLastRef()
STDERR: 18  0x759808 WebCore::TreeShared<WebCore::Node, WebCore::ContainerNode>::deref()
STDERR: 19  0xcb1361 WebCore::V8HTMLDocument::derefObject(void*)
STDERR: 20  0x25bf168 WebCore::WrapperTypeInfo::derefObject(void*)
STDERR: 21  0x25be7a9 WebCore::DOMDataStore::weakCallback(v8::Persistent<v8::Value>, void*)
STDERR: 22  0x8b47881 v8::internal::GlobalHandles::Node::PostGarbageCollectionProcessing(v8::internal::Isolate*, v8::internal::GlobalHandles*)
STDERR: 23  0x8b45812 v8::internal::GlobalHandles::PostGarbageCollectionProcessing(v8::internal::GarbageCollector, v8::internal::GCTracer*)
STDERR: 24  0x8b5e10f v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::internal::GCTracer*)
STDERR: 25  0x8b5d839 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollector, char const*, char const*)
STDERR: 26  0x8a7444b v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, char const*)
STDERR: 27  0x8b5e71b _ZN2v88internalL40AbortIncrementalMarkingAndCollectGarbageEPNS0_4HeapENS0_15AllocationSpaceEPKc
STDERR: 28  0x8b5e5fc v8::internal::Heap::ReserveSpace(int*, unsigned char**)
STDERR: 29  0x8eb3246 v8::internal::Deserializer::DeserializePartial(v8::internal::Object**)
STDERR: 30  0x8ebe0e0 v8::internal::Snapshot::NewContextFromSnapshot()
STDERR: 31  0x8a3ff9d v8::internal::Genesis::Genesis(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::Handle<v8::ObjectTemplate>, v8::ExtensionConfiguration*)
STDERR: Received signal 11

[Elliott Sprehn]

(In reply to comment #15)
> (In reply to comment #14)
> > This also broke the Apple Windows port build for some unknown reason.
> 
> STDOUT: <empty>
> STDERR: ASSERTION FAILED: scope
> STDERR: /Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../html/HTMLStyleElement.cpp(160) : void WebCore::HTMLStyleElement::unregisterWithScopingNode(WebCore::ContainerNode *)
> STDERR: 1   0x1d74190 

Woops, this is because of the bug noticed a few weeks ago where we break the tree scope of shadow roots during removal by adopting them into the document. While usually shadowRoot->treeScope() == shadowRoot, inside of removeAllShadowRoots() we document()->adoptIfNeeded(shadowRoot) which makes shadowRoot->treeScope() == document() instead.

I think I have a fix, running all the tests now.

[Elliott Sprehn]

Created attachment 180538 [details]
Patch

[Elliott Sprehn]

(In reply to comment #17)
> Created an attachment (id=180538) [details]
> Patch

I ran all the tests and the crashes went away.

[Elliott Sprehn]

(In reply to comment #18)
> (In reply to comment #17)
> > Created an attachment (id=180538) [details] [details]
> > Patch
> 
> I ran all the tests and the crashes went away.

Staring at this for longer we should have oldRoot->setParentTreeScope(shadowHost->document()); where the adopt used to be. It seems we always had a bug where the parent tree scope ptr could have been freed, but I can't figure out a way to exploit that since you'd need to make us go through related target adjustment on a ShadowRoot that's had it's host collected.

I'll add that line and upload with cq+.

[Elliott Sprehn]

Created attachment 180565 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 180565 [details]
Patch for landing

Clearing flags on attachment: 180565

Committed r138404: <http://trac.webkit.org/changeset/138404>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.