Bug 197137 - REGRESSION (r243137): SVGViewElement.viewTarget should not return a new object
Summary: REGRESSION (r243137): SVGViewElement.viewTarget should not return a new object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-20 01:30 PDT by Said Abou-Hallawa
Modified: 2019-04-22 11:18 PDT (History)
4 users (show)

See Also:

Attachments
Patch (4.75 KB, patch)
2019-04-20 01:39 PDT, Said Abou-Hallawa 		no flags Details | Formatted Diff | Diff
Followup patch (2.25 KB, patch)
2019-04-21 18:17 PDT, Said Abou-Hallawa 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2019-04-20 01:30:11 PDT 
Open the attached test case. The following assertion fires:

0x0000000762c6d4c0 in ::WTFCrash() at /Volumes/Data/WebKit/OpenSource/Source/WTF/wtf/Assertions.cpp:305
0x0000000749defb5b in WTFCrashWithInfo(int, char const*, char const*, int) at /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/Assertions.h:566
0x000000074b159905 in std::__1::enable_if<std::is_same<WebCore::SVGStringList, WebCore::SVGStringList>::value, WebCore::JSDOMWrapperConverterTraits<WebCore::SVGStringList>::WrapperClass*>::type WebCore::createWrapper<WebCore::SVGStringList, WebCore::SVGStringList>(WebCore::JSDOMGlobalObject*, WTF::Ref<WebCore::SVGStringList, WTF::DumbPtrTraits<WebCore::SVGStringList> >&&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/bindings/js/JSDOMWrapperCache.h:185
0x000000074b1597ec in WebCore::toJSNewlyCreated(JSC::ExecState*, WebCore::JSDOMGlobalObject*, WTF::Ref<WebCore::SVGStringList, WTF::DumbPtrTraits<WebCore::SVGStringList> >&&) at /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/JSSVGStringList.cpp:381
0x000000074b194500 in JSC::JSValue WebCore::JSConverter<WebCore::IDLInterface<WebCore::SVGStringList> >::convertNewlyCreated<WTF::Ref<WebCore::SVGStringList, WTF::DumbPtrTraits<WebCore::SVGStringList> > >(JSC::ExecState&, WebCore::JSDOMGlobalObject&, WTF::Ref<WebCore::SVGStringList, WTF::DumbPtrTraits<WebCore::SVGStringList> >&&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/bindings/js/JSDOMConvertInterface.h:87
0x000000074b1944a0 in JSC::JSValue WebCore::toJSNewlyCreated<WebCore::IDLInterface<WebCore::SVGStringList>, WTF::Ref<WebCore::SVGStringList, WTF::DumbPtrTraits<WebCore::SVGStringList> > >(JSC::ExecState&, WebCore::JSDOMGlobalObject&, WTF::Ref<WebCore::SVGStringList, WTF::DumbPtrTraits<WebCore::SVGStringList> >&&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/bindings/js/JSDOMConvertBase.h:162
0x000000074b194414 in std::__1::enable_if<!(IsExceptionOr<WTF::Ref<WebCore::SVGStringList, WTF::DumbPtrTraits<WebCore::SVGStringList> > >::value), JSC::JSValue>::type WebCore::toJSNewlyCreated<WebCore::IDLInterface<WebCore::SVGStringList>, WTF::Ref<WebCore::SVGStringList, WTF::DumbPtrTraits<WebCore::SVGStringList> > >(JSC::ExecState&, WebCore::JSDOMGlobalObject&, JSC::ThrowScope&, WTF::Ref<WebCore::SVGStringList, WTF::DumbPtrTraits<WebCore::SVGStringList> >&&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/bindings/js/JSDOMConvertBase.h:207
0x000000074b1942d3 in WebCore::jsSVGViewElementViewTargetGetter(JSC::ExecState&, WebCore::JSSVGViewElement&, JSC::ThrowScope&) at /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/JSSVGViewElement.cpp:198
0x000000074b186750 in long long WebCore::IDLAttribute<WebCore::JSSVGViewElement>::get<&(WebCore::jsSVGViewElementViewTargetGetter(JSC::ExecState&, WebCore::JSSVGViewElement&, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)3>(JSC::ExecState&, long long, char const*) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/bindings/js/JSDOMAttribute.h:69
0x000000074b186638 in WebCore::jsSVGViewElementViewTarget(JSC::ExecState*, long long, JSC::PropertyName) at /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/JSSVGViewElement.cpp:204

r243137 removes the tear off objects of SVGStringList. Since the SVGElements now owns Ref pointers to the SVG properties, there is no need to create wrappers for these properties anymore. Therefore all the DOM objects accessing the same property should wrap the same Ref pointer.
Comment 1 Said Abou-Hallawa 2019-04-20 01:39:34 PDT 
Created attachment 367882 [details]
Patch
Comment 2 Said Abou-Hallawa 2019-04-20 01:42:15 PDT 
<rdar://problem/50065198>
Comment 3 Said Abou-Hallawa 2019-04-20 01:48:07 PDT 
Notes:

1. The new layout test, in the patch https://bugs.webkit.org/attachment.cgi?id=367882&action=review, does not pass with the tear off objects implementation. The attribute was not synchronized after changing the SVGStringList.

2. SVGViewElement.viewTarget was removed from SVG 2; see https://www.w3.org/TR/SVG2/linking.html#InterfaceSVGViewElement.
Comment 4 WebKit Commit Bot 2019-04-20 17:03:50 PDT 
Comment on attachment 367882 [details]
Patch

Clearing flags on attachment: 367882

Committed r244491: <https://trac.webkit.org/changeset/244491>
Comment 5 WebKit Commit Bot 2019-04-20 17:03:52 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2019-04-20 17:06:41 PDT 
Comment on attachment 367882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367882&action=review

> Source/WebCore/svg/SVGViewElement.idl:27
> -    [NewObject] readonly attribute SVGStringList viewTarget;
> +    readonly attribute SVGStringList viewTarget;

Wait, had one thought. Should this be using [SameObject] now?
Comment 7 Said Abou-Hallawa 2019-04-21 18:15:37 PDT 
Reopen to upload a followup patch.
Comment 8 Said Abou-Hallawa 2019-04-21 18:17:33 PDT 
Created attachment 367922 [details]
Followup patch
Comment 9 Said Abou-Hallawa 2019-04-22 11:18:08 PDT 
Committed r244503: <https://trac.webkit.org/changeset/244503>