Bug 170454 – Do not assert when CharacterData representing an Attr fires events

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

New assertions were added in <http://trac.webkit.org/changeset/211965> to guard against events being fired at invalid moments during loading and other processing.

After living with the changes for some time, we realized that the assertion was to strict in the case of CharacterData representing a DOM Attribute.

This patch relaxes the conditions under which the assertion is held so that we do not generate false asserts during testing.

[Brent Fulgham]

<rdar://problem/30979320>

[Brent Fulgham]

Created attachment 306178 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 306178 [details]
Patch

You have to wrap the both statements in if !DISABLED_ASSERT because we don't want to be allocating the object in release builds.

[Brent Fulgham]

(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 306178 [details]
> Patch
> 
> You have to wrap the both statements in if !DISABLED_ASSERT because we don't
> want to be allocating the object in release builds.

Is that because I made it a uniqueptr? Or should it have been !ASSERT_DISABLED the entire time?

[Brent Fulgham]

Created attachment 306180 [details]
Patch

[Brent Fulgham]

Created attachment 306185 [details]
Patch

[Chris Dumez]

Comment on attachment 306185 [details]
Patch

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

> LayoutTests/fast/dom/no-assert-for-malformed-js-url-attribute.html:14
> +    testFrame.getAttributeNode("src").firstChild.appendData("javascript: missingFunction(this) orem ipsum dosolorem");

Does this really cover the assertion? I would be surprised since the resulting URL would be "foojavascript:..." which is not a javascript URL.

[Chris Dumez]

Comment on attachment 306185 [details]
Patch

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

>> LayoutTests/fast/dom/no-assert-for-malformed-js-url-attribute.html:14
>> +    testFrame.getAttributeNode("src").firstChild.appendData("javascript: missingFunction(this) orem ipsum dosolorem");
> 
> Does this really cover the assertion? I would be surprised since the resulting URL would be "foojavascript:..." which is not a javascript URL.

We may want to keep both tests. This one crashes as well but the backtrace is slightly different given that it is not treated as a javascript URL:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000117bc9ee4 WTFCrash + 36
1   com.apple.JavaScriptCore      	0x0000000117bc9ef9 WTFCrashWithSecurityImplication + 9
2   com.apple.WebCore             	0x000000010aec7ddf WebCore::EventTarget::fireEventListeners(WebCore::Event&) + 79
3   com.apple.WebCore             	0x000000010adce903 WebCore::DOMWindow::dispatchEvent(WebCore::Event&, WebCore::EventTarget*) + 435
4   com.apple.WebCore             	0x000000010b08acbb WebCore::FrameLoader::dispatchUnloadEvents(WebCore::UnloadEventPolicy) + 379
5   com.apple.WebCore             	0x000000010b08aa65 WebCore::FrameLoader::stopLoading(WebCore::UnloadEventPolicy) + 133
6   com.apple.WebCore             	0x000000010c588459 WebCore::NavigationScheduler::schedule(std::__1::unique_ptr<WebCore::ScheduledNavigation, std::__1::default_delete<WebCore::ScheduledNavigation> >) + 281
7   com.apple.WebCore             	0x000000010c588ed5 WebCore::NavigationScheduler::scheduleLocationChange(WebCore::Document&, WebCore::SecurityOrigin&, WebCore::URL const&, WTF::String const&, WebCore::LockHistory, WebCore::LockBackForwardList) + 1765
8   com.apple.WebCore             	0x000000010cea7117 WebCore::SubframeLoader::loadOrRedirectSubframe(WebCore::HTMLFrameOwnerElement&, WebCore::URL const&, WTF::AtomicString const&, WebCore::LockHistory, WebCore::LockBackForwardList) + 263
9   com.apple.WebCore             	0x000000010cea6e9e WebCore::SubframeLoader::requestFrame(WebCore::HTMLFrameOwnerElement&, WTF::String const&, WTF::AtomicString const&, WebCore::LockHistory, WebCore::LockBackForwardList) + 350
10  com.apple.WebCore             	0x000000010b2ad611 WebCore::HTMLFrameElementBase::openURL(WebCore::LockHistory, WebCore::LockBackForwardList) + 257
11  com.apple.WebCore             	0x000000010b2ad9b9 WebCore::HTMLFrameElementBase::setLocation(WTF::String const&) + 185
12  com.apple.WebCore             	0x000000010b2ad6e1 WebCore::HTMLFrameElementBase::parseAttribute(WebCore::QualifiedName const&, WTF::AtomicString const&) + 193
13  com.apple.WebCore             	0x000000010b2b2c09 WebCore::HTMLIFrameElement::parseAttribute(WebCore::QualifiedName const&, WTF::AtomicString const&) + 393
14  com.apple.WebCore             	0x000000010ae5e4d9 WebCore::Element::attributeChanged(WebCore::QualifiedName const&, WTF::AtomicString const&, WTF::AtomicString const&, WebCore::Element::AttributeModificationReason) + 793
15  com.apple.WebCore             	0x000000010ce2aa2f WebCore::StyledElement::attributeChanged(WebCore::QualifiedName const&, WTF::AtomicString const&, WTF::AtomicString const&, WebCore::Element::AttributeModificationReason) + 223

[Brent Fulgham]

Created attachment 306187 [details]
Patch

[Build Bot]

Comment on attachment 306187 [details]
Patch

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

New failing tests:
fast/dom/no-assert-for-malformed-js-url-attribute.html

[Build Bot]

Created attachment 306197 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 306187 [details]
Patch

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

New failing tests:
fast/dom/no-assert-for-malformed-js-url-attribute.html

[Build Bot]

Created attachment 306198 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Brent Fulgham]

Look like my revised test output is wrong. I'll rebase before landing.

[Build Bot]

Comment on attachment 306187 [details]
Patch

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

New failing tests:
fast/dom/no-assert-for-malformed-js-url-attribute.html

[Build Bot]

Created attachment 306200 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 306187 [details]
Patch

Attachment 306187 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3473377

New failing tests:
fast/dom/no-assert-for-malformed-js-url-attribute.html

[Build Bot]

Created attachment 306208 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Brent Fulgham]

Created attachment 306225 [details]
Patch for Landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 306225 [details]:

media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-styles.html bug 168317 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 306225 [details]
Patch for Landing

Clearing flags on attachment: 306225

Committed r214915: <http://trac.webkit.org/changeset/214915>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.