Bug 105345 - Replace documentFragmentIsShadowRoot with isTreeScope
Summary: Replace documentFragmentIsShadowRoot with isTreeScope
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on: 105621
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-18 13:55 PST by Elliott Sprehn
Modified: 2012-12-21 16:26 PST (History)
9 users (show)

See Also:


Attachments
Patch (3.46 KB, patch)
2012-12-18 14:14 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (4.01 KB, patch)
2012-12-18 14:41 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (4.71 KB, patch)
2012-12-20 16:14 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (7.21 KB, patch)
2012-12-20 17:10 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (9.47 KB, patch)
2012-12-21 11:27 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (9.62 KB, patch)
2012-12-21 15:50 PST, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2012-12-18 13:55:26 PST
Replace documentFragmentIsShadowRoot with isTreeScope
Comment 1 Elliott Sprehn 2012-12-18 14:14:17 PST
Created attachment 180022 [details]
Patch
Comment 2 Elliott Sprehn 2012-12-18 14:41:26 PST
Created attachment 180030 [details]
Patch
Comment 3 Build Bot 2012-12-18 15:36:49 PST
Comment on attachment 180030 [details]
Patch

Attachment 180030 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15413296
Comment 4 Elliott Sprehn 2012-12-18 18:03:42 PST
(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.
Comment 5 Build Bot 2012-12-18 18:59:40 PST
Comment on attachment 180030 [details]
Patch

Attachment 180030 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15399493
Comment 6 Elliott Sprehn 2012-12-20 16:14:34 PST
Created attachment 180432 [details]
Patch
Comment 7 Build Bot 2012-12-20 16:48:20 PST
Comment on attachment 180432 [details]
Patch

Attachment 180432 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15460128
Comment 8 Elliott Sprehn 2012-12-20 17:10:26 PST
Created attachment 180445 [details]
Patch
Comment 9 Dimitri Glazkov (Google) 2012-12-20 18:17:32 PST
Comment on attachment 180445 [details]
Patch

You can also just do webkit-patch land-safely. You're a committer now, right?
Comment 10 Elliott Sprehn 2012-12-20 18:20:10 PST
(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. :)
Comment 11 WebKit Review Bot 2012-12-20 18:43:19 PST
Comment on attachment 180445 [details]
Patch

Clearing flags on attachment: 180445

Committed r138338: <http://trac.webkit.org/changeset/138338>
Comment 12 WebKit Review Bot 2012-12-20 18:43:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 WebKit Review Bot 2012-12-21 02:02:34 PST
Re-opened since this is blocked by bug 105621
Comment 14 Tim Horton 2012-12-21 02:16:14 PST
This also broke the Apple Windows port build for some unknown reason.
Comment 15 Ilya Tikhonovsky 2012-12-21 07:08:51 PST
(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
Comment 16 Elliott Sprehn 2012-12-21 09:56:17 PST
(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.
Comment 17 Elliott Sprehn 2012-12-21 11:27:29 PST
Created attachment 180538 [details]
Patch
Comment 18 Elliott Sprehn 2012-12-21 11:44:23 PST
(In reply to comment #17)
> Created an attachment (id=180538) [details]
> Patch

I ran all the tests and the crashes went away.
Comment 19 Elliott Sprehn 2012-12-21 15:15:48 PST
(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+.
Comment 20 Elliott Sprehn 2012-12-21 15:50:32 PST
Created attachment 180565 [details]
Patch for landing
Comment 21 WebKit Review Bot 2012-12-21 16:26:20 PST
Comment on attachment 180565 [details]
Patch for landing

Clearing flags on attachment: 180565

Committed r138404: <http://trac.webkit.org/changeset/138404>
Comment 22 WebKit Review Bot 2012-12-21 16:26:25 PST
All reviewed patches have been landed.  Closing bug.