Bug 151931 – Reduce the number of events that can be created by Document.createEvent

• Component: DOM
• Status: RESOLVED FIXED

[Darin Adler]

Reduce the number of events that can be created by Document.createEvent

[Darin Adler]

Created attachment 266753 [details]
Patch

[Darin Adler]

Comment on attachment 266753 [details]
Patch

I started trying to fix the build. I think I know ow to do it for DerivedSources.make, which should fix Windows and Mac. No idea how to fix it in CMake, though.

[Darin Adler]

Created attachment 266891 [details]
Patch

[Darin Adler]

Created attachment 266896 [details]
Patch

[Build Bot]

Comment on attachment 266896 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TrackEvent/createEvent.html
animations/animation-events-create.html
fast/events/init-events.html
fast/events/event-creation.html
storage/indexeddb/removed.html
svg/custom/immutable-properties.html
indieui/create-uirequestevent.html
imported/w3c/web-platform-tests/dom/events/ProgressEvent.html
transitions/transition-end-event-create.html

[Build Bot]

Created attachment 266903 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 266896 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TrackEvent/createEvent.html
animations/animation-events-create.html
fast/events/init-events.html
fast/events/event-creation.html
storage/indexeddb/removed.html
svg/custom/immutable-properties.html
indieui/create-uirequestevent.html
imported/w3c/web-platform-tests/dom/events/ProgressEvent.html
transitions/transition-end-event-create.html
storage/indexeddb/events.html

[Build Bot]

Created attachment 266904 [details]
Archive of layout-test-results from ews103 for mac-yosemite

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

[Build Bot]

Comment on attachment 266896 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TrackEvent/createEvent.html
indieui/create-uirequestevent.html
animations/animation-events-create.html
fast/events/init-events.html
storage/indexeddb/removed.html
svg/custom/immutable-properties.html
fast/events/event-creation.html
imported/w3c/web-platform-tests/dom/events/ProgressEvent.html
transitions/transition-end-event-create.html
storage/indexeddb/events.html

[Build Bot]

Created attachment 266912 [details]
Archive of layout-test-results from ews115 for mac-yosemite

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

[Darin Adler]

Created attachment 267063 [details]
Patch

[Alex Christensen]

Comment on attachment 267063 [details]
Patch

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

r=me with a few comments.

> Source/WebCore/dom/Document.cpp:4103
> +    // required for compatibility some actual legacy web content.

compatibility *with* some actual legacy web content.

> Source/WebCore/dom/Document.cpp:4115
> +    if (equalIgnoringASCIICase(type, "keyboardevent") || equalIgnoringASCIICase(type, "keyboardevents"))

The whatwg spec says "keyevents", not "keyboardevents"

> Source/WebCore/dom/Document.cpp:4134
> +    // As we prove there is no content depending on each string we would like to *remove*
> +    // both the string and the corresponding init function for these classes.

What determines whether there is little enough web content to remove the event string?

> Source/WebCore/dom/Document.cpp:4158
> +    // The following strings are for event classes where we do not supply an init function.

There are no following strings.  I don't think this comment is necessary.

> LayoutTests/storage/indexeddb/events-expected.txt:9
> -PASS 'newVersion' in document.createEvent('IDBVersionChangeEvent') is true
> +FAIL 'oldVersion' in document.createEvent('IDBVersionChangeEvent') should be true. Threw exception Error: NotSupportedError: DOM Exception 9

Brady is currently developing indexeddb and is working on making all these tests pass.  I'd rather skip these tests along with many other indexeddb tests that are skipped than change test expectations from PASS to FAIL.  That way we can be sure to look at them as we make all the tests pass.  See the list in LayoutTests/platform/mac-wk1/TestExpectations and some also in LayoutTests/platform/wk2/TestExpectations.

> LayoutTests/storage/indexeddb/removed-expected.txt:14
> -PASS 'version' in document.createEvent('IDBVersionChangeEvent') is false
> +FAIL 'version' in document.createEvent('IDBVersionChangeEvent') should be false. Threw exception Error: NotSupportedError: DOM Exception 9

ditto

> LayoutTests/svg/custom/immutable-properties-expected.txt:10
> -PASS successfullyParsed is true
> +FAIL successfullyParsed should be true (of type boolean). Was undefined (of type undefined).

I'm not about this one.  We certainly do have some places with FAIL in the -expected.txt, but it's not ideal

[Darin Adler]

Comment on attachment 267063 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:4115
>> +    if (equalIgnoringASCIICase(type, "keyboardevent") || equalIgnoringASCIICase(type, "keyboardevents"))
> 
> The whatwg spec says "keyevents", not "keyboardevents"

Very interesting! We have never had support for "keyevents" in the past, so we probably need two FIXMEs: one about adding "keyevents" and another about removing "keyboardevents".

>> Source/WebCore/dom/Document.cpp:4134
>> +    // both the string and the corresponding init function for these classes.
> 
> What determines whether there is little enough web content to remove the event string?

This is the same kind of research we have to do to remove any web-exposed feature, such as removing a DOM function. This is not a particularly special case.

One way to do it is to remove first and then fix problems by waiting to see if bug reports come in. This is what I have already chosen to do in this patch with all the many strings that can be used to create events where there is no way to initialize them and thus make a useful initialized event. Any dependency on those particular strings would be super-shallow since it’s hard to usefully depend on creating an event that you can’t use for anything!

A second approach is to do various types of searches on existing web content. In the past Google has sometimes helped us by doing searches of the content in their cache of copies of most websites on the Internet. We can also try doing searches of WebKit-specific content, such as the content in OS X and iOS apps.

A third approach is to survey the behavior of other browsers. Sometimes we take the risk of being incompatible with older versions of WebKit as long as we are becoming consistent with other widely used browser engines, hoping to at least preserve compatibility with websites that have tested with more than one engine and don’t special case any engine.

We have a lot of experience doing this with other web-facing features, but I personally am not the one who has done it, so I don’t know exactly the strategy we took.

>> Source/WebCore/dom/Document.cpp:4158
>> +    // The following strings are for event classes where we do not supply an init function.
> 
> There are no following strings.  I don't think this comment is necessary.

OK, I am willing to take it out.

>> LayoutTests/storage/indexeddb/events-expected.txt:9
>> +FAIL 'oldVersion' in document.createEvent('IDBVersionChangeEvent') should be true. Threw exception Error: NotSupportedError: DOM Exception 9
> 
> Brady is currently developing indexeddb and is working on making all these tests pass.  I'd rather skip these tests along with many other indexeddb tests that are skipped than change test expectations from PASS to FAIL.  That way we can be sure to look at them as we make all the tests pass.  See the list in LayoutTests/platform/mac-wk1/TestExpectations and some also in LayoutTests/platform/wk2/TestExpectations.

The test is flawed. It specifically is checking if you can create an IDBVersionChangeEvent with createEvent, and you should not be able to do so. Instead the test should create the event with the constructor.

The right thing to do is to add the event constructor. This is specified in http://www.w3.org/TR/IndexedDB/ and so it’s quite reasonable for us to do this. And then change this test to add the event constructor. I guess I can do that all in this patch, but my original plan was to check in with part of the test failing and then fix it.

Skipping the test seems like a bad idea. It’s still doing useful testing of everything except for createEvent. I personally prefer expecting failures to not running tests at all, in almost every case. I’d love to hear Brady’s take on this.

>> LayoutTests/svg/custom/immutable-properties-expected.txt:10
>> +FAIL successfullyParsed should be true (of type boolean). Was undefined (of type undefined).
> 
> I'm not about this one.  We certainly do have some places with FAIL in the -expected.txt, but it's not ideal

We have multiple options:

1) Find a way to make the engine generate an SVGZoomEvent by making the test actually zoom. Use that to test the properties are immutable.
2) Add a constructor for SVGZoomEvent. Change this test to use that.
3) Add a way to create an SVGZoomEvent that is just for testing, perhaps a createEvent function on the internals object.
4) Don't bother testing that SVGZoomEvent properties are immutable forever, or until we do (1), (2), or (3).

The only one of these I don’t want to do at this time is (2), because it seems inappropriate for me to make a web-platform-visible change just to keep this marginally useful test working.

[Brady Eidson]

(In reply to comment #13)
> Comment on attachment 267063 [details]
> 
> >> LayoutTests/storage/indexeddb/events-expected.txt:9
> >> +FAIL 'oldVersion' in document.createEvent('IDBVersionChangeEvent') should be true. Threw exception Error: NotSupportedError: DOM Exception 9
> > 
> > Brady is currently developing indexeddb and is working on making all these tests pass.  I'd rather skip these tests along with many other indexeddb tests that are skipped than change test expectations from PASS to FAIL.  That way we can be sure to look at them as we make all the tests pass.  See the list in LayoutTests/platform/mac-wk1/TestExpectations and some also in LayoutTests/platform/wk2/TestExpectations.
> 
> The test is flawed. It specifically is checking if you can create an
> IDBVersionChangeEvent with createEvent, and you should not be able to do so.
> Instead the test should create the event with the constructor.
> 
> The right thing to do is to add the event constructor. This is specified in
> http://www.w3.org/TR/IndexedDB/ and so it’s quite reasonable for us to do
> this. And then change this test to add the event constructor. I guess I can
> do that all in this patch, but my original plan was to check in with part of
> the test failing and then fix it.
> 
> Skipping the test seems like a bad idea. It’s still doing useful testing of
> everything except for createEvent. I personally prefer expecting failures to
> not running tests at all, in almost every case. I’d love to hear Brady’s
> take on this.

I would rather the expectations be left as-is, and then for the tests to be added to LayoutTests/platform/mac-wk1/TestExpectations and marked as [ Failure ].

That way the test is still run, and we'd still catch more critical regressions (crash, timeout) as IDB development continues, but the disagreement about the results won't hold anybody up for now.

That will also automatically gets placed in my "these tests need attention" bucket, and I will actively working on that bucket for the foreseeable future (https://bugs.webkit.org/show_bug.cgi?id=150882).

I'm sure it'll be easy to decide the right course of action at a later time in a patch that is only about those tests.

[Darin Adler]

Comment on attachment 267063 [details]
Patch

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

>>>> LayoutTests/storage/indexeddb/events-expected.txt:9
>>>> +FAIL 'oldVersion' in document.createEvent('IDBVersionChangeEvent') should be true. Threw exception Error: NotSupportedError: DOM Exception 9
>>> 
>>> Brady is currently developing indexeddb and is working on making all these tests pass.  I'd rather skip these tests along with many other indexeddb tests that are skipped than change test expectations from PASS to FAIL.  That way we can be sure to look at them as we make all the tests pass.  See the list in LayoutTests/platform/mac-wk1/TestExpectations and some also in LayoutTests/platform/wk2/TestExpectations.
>> 
>> The test is flawed. It specifically is checking if you can create an IDBVersionChangeEvent with createEvent, and you should not be able to do so. Instead the test should create the event with the constructor.
>> 
>> The right thing to do is to add the event constructor. This is specified in http://www.w3.org/TR/IndexedDB/ and so it’s quite reasonable for us to do this. And then change this test to add the event constructor. I guess I can do that all in this patch, but my original plan was to check in with part of the test failing and then fix it.
>> 
>> Skipping the test seems like a bad idea. It’s still doing useful testing of everything except for createEvent. I personally prefer expecting failures to not running tests at all, in almost every case. I’d love to hear Brady’s take on this.
> 
> I would rather the expectations be left as-is, and then for the tests to be added to LayoutTests/platform/mac-wk1/TestExpectations and marked as [ Failure ].
> 
> That way the test is still run, and we'd still catch more critical regressions (crash, timeout) as IDB development continues, but the disagreement about the results won't hold anybody up for now.
> 
> That will also automatically gets placed in my "these tests need attention" bucket, and I will actively working on that bucket for the foreseeable future (https://bugs.webkit.org/show_bug.cgi?id=150882).
> 
> I'm sure it'll be easy to decide the right course of action at a later time in a patch that is only about those tests.

OK, but please do be aware that those expectations are wrong. The test itself is incorrect; and this is not really a “disagreement” since it’s completely clear in the IDB specification. The other parts of the test are not incorrect, it could catch problems other than crashes or hangs if we didn’t expect failure on the test as a whole.

[Darin Adler]

I’ll make a new patch that takes into account Alex’s and Brady’s comments. But I’m not sure yet what to do about SVGZoomEvent; I would appreciate help making the call on that.

[Brady Eidson]

(In reply to comment #15)
> Comment on attachment 267063 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267063&action=review
> 
> >>>> LayoutTests/storage/indexeddb/events-expected.txt:9
> >>>> +FAIL 'oldVersion' in document.createEvent('IDBVersionChangeEvent') should be true. Threw exception Error: NotSupportedError: DOM Exception 9
> >>> 
> >>> Brady is currently developing indexeddb and is working on making all these tests pass.  I'd rather skip these tests along with many other indexeddb tests that are skipped than change test expectations from PASS to FAIL.  That way we can be sure to look at them as we make all the tests pass.  See the list in LayoutTests/platform/mac-wk1/TestExpectations and some also in LayoutTests/platform/wk2/TestExpectations.
> >> 
> >> The test is flawed. It specifically is checking if you can create an IDBVersionChangeEvent with createEvent, and you should not be able to do so. Instead the test should create the event with the constructor.
> >> 
> >> The right thing to do is to add the event constructor. This is specified in http://www.w3.org/TR/IndexedDB/ and so it’s quite reasonable for us to do this. And then change this test to add the event constructor. I guess I can do that all in this patch, but my original plan was to check in with part of the test failing and then fix it.
> >> 
> >> Skipping the test seems like a bad idea. It’s still doing useful testing of everything except for createEvent. I personally prefer expecting failures to not running tests at all, in almost every case. I’d love to hear Brady’s take on this.
> > 
> > I would rather the expectations be left as-is, and then for the tests to be added to LayoutTests/platform/mac-wk1/TestExpectations and marked as [ Failure ].
> > 
> > That way the test is still run, and we'd still catch more critical regressions (crash, timeout) as IDB development continues, but the disagreement about the results won't hold anybody up for now.
> > 
> > That will also automatically gets placed in my "these tests need attention" bucket, and I will actively working on that bucket for the foreseeable future (https://bugs.webkit.org/show_bug.cgi?id=150882).
> > 
> > I'm sure it'll be easy to decide the right course of action at a later time in a patch that is only about those tests.
> 
> OK, but please do be aware that those expectations are wrong. The test
> itself is incorrect; and this is not really a “disagreement” since it’s
> completely clear in the IDB specification. The other parts of the test are
> not incorrect, it could catch problems other than crashes or hangs if we
> didn’t expect failure on the test as a whole.

I wasn't clear. I know the test is wrong. I meant the disagreement being about what to do with the situation RIGHT NOW. 

If the test ends up in TestExpectations as a [ Failure ], then it will not be ignored.

[Darin Adler]

(In reply to comment #17)
> If the test ends up in TestExpectations as a [ Failure ], then it will not
> be ignored.

I have already done what you requested and added these to TestExpectations with [Failure] and that’s what I will land at your request, since that fits in with how you are working on IndexedDB.

However, I think that this concept we inherited from Google, of tests that we run where we expect the results to be failures, is not a great one.

There is a strength of this concept: all the tests are listed in TestExpectations, so it's practical to use that as a work list to improve test behavior. But there is a major a weakness: any parts of the test that are still working correctly are no longer checked at all. So once we set an expectation to [Failure], everything else in the test is run but the output is not checked at all; thus we are still screening for crashes, but otherwise the test is not providing any value.

The original point of the Skipped file (predecessor to TestExpectations) was to make it practical to keep tests around that currently causes crashes or cause problems with other tests. The strategy for failures was always to update expected results to expect the particular failures that existed at that point in time.

I can see how it’s useful to have a work list of failures to fix. So perhaps we should create a new concept where the test runner script will understand the text FAIL in test output and consider that a failure. Thus we could still land an expected file with the known failure in it, and there would still need to be an item in the TestExpectations file that someone could use to plan their work, but the item would be different than the "just expect a failure, anything goes other than success" that [Failure] currently means. Not sure what the right name would be.

Right now we have way too many items in TextExpectations files; I suppose my proposal would make us need even more!

[Darin Adler]

Committed r193957: <http://trac.webkit.org/changeset/193957>

[Radar WebKit Bug Importer]

<rdar://problem/23872168>

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.

[Anne van Kesteren]

*** Bug 103423 has been marked as a duplicate of this bug. ***