Bug 170454

Summary:

Do not assert when CharacterData representing an Attr fires events

Product:

WebKit

Reporter:

Brent Fulgham <bfulgham>

Component:

WebCore Misc.

Assignee:

Brent Fulgham <bfulgham>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, buildbot, cdumez, commit-queue, dbates, esprehn+autocc, kangil.han, rniwa

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2	none
Archive of layout-test-results from ews103 for mac-elcapitan	none
Archive of layout-test-results from ews117 for mac-elcapitan	none
Archive of layout-test-results from ews124 for ios-simulator-wk2	none
Patch for Landing	none

Brent Fulgham

Reported 2017-04-04 09:52:05 PDT

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.

Attachments
Patch (1.82 KB, patch) 2017-04-04 10:47 PDT, Brent Fulgham	no flags	Details Formatted Diff Diff
Patch (1.88 KB, patch) 2017-04-04 11:08 PDT, Brent Fulgham	no flags	Details Formatted Diff Diff
Patch (4.24 KB, patch) 2017-04-04 12:27 PDT, Brent Fulgham	no flags	Details Formatted Diff Diff
Patch (4.53 KB, patch) 2017-04-04 12:56 PDT, Brent Fulgham	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.13 MB, application/zip) 2017-04-04 13:56 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews103 for mac-elcapitan (908.34 KB, application/zip) 2017-04-04 14:01 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.58 MB, application/zip) 2017-04-04 14:13 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (870.34 KB, application/zip) 2017-04-04 14:29 PDT, Build Bot	no flags	Details
Patch for Landing (4.58 KB, patch) 2017-04-04 16:12 PDT, Brent Fulgham	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Brent Fulgham

Comment 1 2017-04-04 09:52:20 PDT

<rdar://problem/30979320>

Brent Fulgham

Comment 2 2017-04-04 10:47:14 PDT

Created attachment 306178 [details] Patch

Ryosuke Niwa

Comment 3 2017-04-04 10:56:52 PDT

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

Comment 4 2017-04-04 11:07:42 PDT

(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

Comment 5 2017-04-04 11:08:01 PDT

Created attachment 306180 [details] Patch

Brent Fulgham

Comment 6 2017-04-04 12:27:37 PDT

Created attachment 306185 [details] Patch

Chris Dumez

Comment 7 2017-04-04 12:36:10 PDT

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 8 2017-04-04 12:48:26 PDT

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

Comment 9 2017-04-04 12:56:37 PDT

Created attachment 306187 [details] Patch

Build Bot

Comment 10 2017-04-04 13:56:07 PDT

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

Comment 11 2017-04-04 13:56:08 PDT

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 12 2017-04-04 14:01:01 PDT

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

Comment 13 2017-04-04 14:01:02 PDT

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

Comment 14 2017-04-04 14:05:35 PDT

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

Build Bot

Comment 15 2017-04-04 14:13:32 PDT

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

Comment 16 2017-04-04 14:13:33 PDT

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 17 2017-04-04 14:29:53 PDT

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

Comment 18 2017-04-04 14:29:55 PDT

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

Comment 19 2017-04-04 16:12:56 PDT

Created attachment 306225 [details] Patch for Landing

WebKit Commit Bot

Comment 20 2017-04-04 17:05:02 PDT

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: graouts@apple.com) The commit-queue is continuing to process your patch.

WebKit Commit Bot

Comment 21 2017-04-04 17:05:30 PDT

Comment on attachment 306225 [details] Patch for Landing Clearing flags on attachment: 306225 Committed r214915: <http://trac.webkit.org/changeset/214915>

WebKit Commit Bot

Comment 22 2017-04-04 17:05:32 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.