Bug 170454 - Do not assert when CharacterData representing an Attr fires events
Summary: Do not assert when CharacterData representing an Attr fires events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-04 09:52 PDT by Brent Fulgham
Modified: 2017-04-04 17:05 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 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.
Comment 1 Brent Fulgham 2017-04-04 09:52:20 PDT
<rdar://problem/30979320>
Comment 2 Brent Fulgham 2017-04-04 10:47:14 PDT
Created attachment 306178 [details]
Patch
Comment 3 Ryosuke Niwa 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.
Comment 4 Brent Fulgham 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?
Comment 5 Brent Fulgham 2017-04-04 11:08:01 PDT
Created attachment 306180 [details]
Patch
Comment 6 Brent Fulgham 2017-04-04 12:27:37 PDT
Created attachment 306185 [details]
Patch
Comment 7 Chris Dumez 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.
Comment 8 Chris Dumez 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
Comment 9 Brent Fulgham 2017-04-04 12:56:37 PDT
Created attachment 306187 [details]
Patch
Comment 10 Build Bot 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
Comment 11 Build Bot 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
Comment 12 Build Bot 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
Comment 13 Build Bot 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
Comment 14 Brent Fulgham 2017-04-04 14:05:35 PDT
Look like my revised test output is wrong. I'll rebase before landing.
Comment 15 Build Bot 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
Comment 16 Build Bot 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
Comment 17 Build Bot 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
Comment 18 Build Bot 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
Comment 19 Brent Fulgham 2017-04-04 16:12:56 PDT
Created attachment 306225 [details]
Patch for Landing
Comment 20 WebKit Commit Bot 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.
Comment 21 WebKit Commit Bot 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>
Comment 22 WebKit Commit Bot 2017-04-04 17:05:32 PDT
All reviewed patches have been landed.  Closing bug.