Bug 179591

Summary: Event improvements
Product: WebKit Reporter: Darin Adler <darin>
Component: WebCore JavaScriptAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, buildbot, cdumez, ews-watchlist, graouts, rniwa, ross.kirsling, sam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=182603
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews101 for mac-sierra
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Archive of layout-test-results from ews117 for mac-sierra
none
Patch
none
Archive of layout-test-results from ews100 for mac-sierra
none
Archive of layout-test-results from ews115 for mac-sierra
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Patch none

Description Darin Adler 2017-11-11 22:32:40 PST 
Event improvements
Comment 1 Darin Adler 2017-11-11 23:37:32 PST Comment hidden (obsolete)
Comment 2 Darin Adler 2017-11-12 13:10:58 PST Comment hidden (obsolete)
Comment 3 Build Bot 2017-11-12 13:12:17 PST Comment hidden (obsolete)
Comment 4 Build Bot 2017-11-12 13:15:01 PST Comment hidden (obsolete)
Comment 5 Build Bot 2017-11-12 14:12:45 PST Comment hidden (obsolete)
Comment 6 Build Bot 2017-11-12 14:12:47 PST Comment hidden (obsolete)
Comment 7 Build Bot 2017-11-12 14:23:16 PST Comment hidden (obsolete)
Comment 8 Build Bot 2017-11-12 14:23:18 PST Comment hidden (obsolete)
Comment 9 Build Bot 2017-11-12 14:35:46 PST Comment hidden (obsolete)
Comment 10 Build Bot 2017-11-12 14:35:47 PST Comment hidden (obsolete)
Comment 11 Darin Adler 2018-01-13 12:16:28 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2018-01-13 12:19:59 PST Comment hidden (obsolete)
Comment 13 EWS Watchlist 2018-01-13 12:21:04 PST Comment hidden (obsolete)
Comment 14 Darin Adler 2018-01-13 12:28:14 PST Comment hidden (obsolete)
Comment 15 EWS Watchlist 2018-01-13 12:31:18 PST Comment hidden (obsolete)
Comment 16 EWS Watchlist 2018-01-13 12:32:00 PST Comment hidden (obsolete)
Comment 17 Darin Adler 2018-01-13 12:59:20 PST Comment hidden (obsolete)
Comment 18 EWS Watchlist 2018-01-13 13:01:25 PST Comment hidden (obsolete)
Comment 19 Darin Adler 2018-01-13 13:20:27 PST Comment hidden (obsolete)
Comment 20 EWS Watchlist 2018-01-13 13:23:03 PST Comment hidden (obsolete)
Comment 21 Darin Adler 2018-01-13 13:49:27 PST Comment hidden (obsolete)
Comment 22 EWS Watchlist 2018-01-13 13:50:49 PST Comment hidden (obsolete)
Comment 23 EWS Watchlist 2018-01-13 14:54:06 PST Comment hidden (obsolete)
Comment 24 EWS Watchlist 2018-01-13 14:54:08 PST Comment hidden (obsolete)
Comment 25 EWS Watchlist 2018-01-13 15:01:14 PST Comment hidden (obsolete)
Comment 26 EWS Watchlist 2018-01-13 15:01:15 PST Comment hidden (obsolete)
Comment 27 EWS Watchlist 2018-01-13 15:17:36 PST Comment hidden (obsolete)
Comment 28 EWS Watchlist 2018-01-13 15:17:37 PST Comment hidden (obsolete)
Comment 29 Darin Adler 2018-01-13 16:37:13 PST Comment hidden (obsolete)
Comment 30 EWS Watchlist 2018-01-13 17:47:28 PST Comment hidden (obsolete)
Comment 31 EWS Watchlist 2018-01-13 17:47:29 PST Comment hidden (obsolete)
Comment 32 EWS Watchlist 2018-01-13 18:17:28 PST Comment hidden (obsolete)
Comment 33 EWS Watchlist 2018-01-13 18:17:30 PST Comment hidden (obsolete)
Comment 34 EWS Watchlist 2018-01-13 19:41:05 PST Comment hidden (obsolete)
Comment 35 EWS Watchlist 2018-01-13 19:41:06 PST Comment hidden (obsolete)
Comment 36 Darin Adler 2018-01-13 20:13:33 PST 
Created attachment 331305 [details]
Patch
Comment 37 EWS Watchlist 2018-01-13 20:15:30 PST 
Attachment 331305 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_wasThrown is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:829:  out_savedResultIndex is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMWheelEvent.cpp"
ERROR: Source/WebCore/dom/KeyboardEvent.h:40:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:41:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/KeyboardEvent.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 6 in 120 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 38 Darin Adler 2018-01-13 21:55:35 PST 
Hooray, all platforms are compiling now and all tests are passing.

I hope that someone will review now and either ask me to cut this down and land it in smaller pieces, or approve this for landing.
Comment 39 Darin Adler 2018-02-01 09:22:42 PST 
Chris, you might be the best reviewer for this. When you have a chance I’d love to hear your thoughts. No real rush, but I did get this working well three weeks ago, so it’s ready for you when you have some time.
Comment 40 Chris Dumez 2018-02-01 16:05:53 PST 
Comment on attachment 331305 [details]
Patch

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

r=me

> Source/WebCore/bindings/js/JSValueInWrappedObject.h:36
> +// FIXME: The following uses of JSC::Strong are incorrect, can lead to storage leaks

We may want those comments to be in those classes. Otherwise, it is easy to forget removing them when those classes get updated (since we presumably wouldn't have to update this file).

> Source/WebCore/bindings/js/JSValueInWrappedObject.h:48
> +// FIXME: Better name for this?

I don't have a better idea.

> Source/WebCore/dom/CustomEvent.h:49
> +    const JSValueInWrappedObject& detail() const { return m_detail; }

I do not quite understand why this class needs both a detail and a cachedDetail? What's the difference between the 2? Why cannot we rely solely on detail()? It seems cachedDetail is basically always initialized from detail.

> Source/WebCore/dom/Event.h:163
> +    bool m_canceled { false };

nice. I know it is called canceled in the spec, but WebKit coding would recommend a prefix here. Maybe m_wasCanceled.
Comment 41 Darin Adler 2018-02-01 20:39:22 PST 
Comment on attachment 331305 [details]
Patch

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

>> Source/WebCore/dom/CustomEvent.h:49
>> +    const JSValueInWrappedObject& detail() const { return m_detail; }
> 
> I do not quite understand why this class needs both a detail and a cachedDetail? What's the difference between the 2? Why cannot we rely solely on detail()? It seems cachedDetail is basically always initialized from detail.

m_cachedDetail can be m_detail cloned across worlds
Comment 42 Chris Dumez 2018-02-02 12:41:24 PST 
(In reply to Darin Adler from comment #41)
> Comment on attachment 331305 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331305&action=review
> 
> >> Source/WebCore/dom/CustomEvent.h:49
> >> +    const JSValueInWrappedObject& detail() const { return m_detail; }
> > 
> > I do not quite understand why this class needs both a detail and a cachedDetail? What's the difference between the 2? Why cannot we rely solely on detail()? It seems cachedDetail is basically always initialized from detail.
> 
> m_cachedDetail can be m_detail cloned across worlds

But it sounds like an optimization, right? We could purely rely on m_detail and clone on demand when needed. I am not sure it is worth optimizing this. I do not even know how a CustomEvent could be passed across world.
Comment 43 Darin Adler 2018-02-02 14:05:56 PST 
Comment on attachment 331305 [details]
Patch

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

>> Source/WebCore/bindings/js/JSValueInWrappedObject.h:36
>> +// FIXME: The following uses of JSC::Strong are incorrect, can lead to storage leaks
> 
> We may want those comments to be in those classes. Otherwise, it is easy to forget removing them when those classes get updated (since we presumably wouldn't have to update this file).

OK, I will do that.

>>>> Source/WebCore/dom/CustomEvent.h:49
>>>> +    const JSValueInWrappedObject& detail() const { return m_detail; }
>>> 
>>> I do not quite understand why this class needs both a detail and a cachedDetail? What's the difference between the 2? Why cannot we rely solely on detail()? It seems cachedDetail is basically always initialized from detail.
>> 
>> m_cachedDetail can be m_detail cloned across worlds
> 
> But it sounds like an optimization, right? We could purely rely on m_detail and clone on demand when needed. I am not sure it is worth optimizing this. I do not even know how a CustomEvent could be passed across world.

The helper that handles the lifetime across worlds called cachedPropertyValue is used two places: JSCustomEvent::detail and JSMessageEvent::data. In the case of JSMessageEvent, I was preserving the existing caching that has reasons to be there besides the "cloned across worlds" issue. I guess you are saying that here in JSCustomEvent the caching isn’t needed or helpful. It’s easy for me to remove it, I suppose. I think I was hoping that the pattern would be more similar in all these classes. I can eliminate this caching.

>> Source/WebCore/dom/Event.h:163
>> +    bool m_canceled { false };
> 
> nice. I know it is called canceled in the spec, but WebKit coding would recommend a prefix here. Maybe m_wasCanceled.

OK, yes I will use m_wasCanceled.
Comment 44 Darin Adler 2018-02-03 09:03:49 PST 
Comment on attachment 331305 [details]
Patch

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

>>>>> Source/WebCore/dom/CustomEvent.h:49
>>>>> +    const JSValueInWrappedObject& detail() const { return m_detail; }
>>>> 
>>>> I do not quite understand why this class needs both a detail and a cachedDetail? What's the difference between the 2? Why cannot we rely solely on detail()? It seems cachedDetail is basically always initialized from detail.
>>> 
>>> m_cachedDetail can be m_detail cloned across worlds
>> 
>> But it sounds like an optimization, right? We could purely rely on m_detail and clone on demand when needed. I am not sure it is worth optimizing this. I do not even know how a CustomEvent could be passed across world.
> 
> The helper that handles the lifetime across worlds called cachedPropertyValue is used two places: JSCustomEvent::detail and JSMessageEvent::data. In the case of JSMessageEvent, I was preserving the existing caching that has reasons to be there besides the "cloned across worlds" issue. I guess you are saying that here in JSCustomEvent the caching isn’t needed or helpful. It’s easy for me to remove it, I suppose. I think I was hoping that the pattern would be more similar in all these classes. I can eliminate this caching.

OK, I figured more about why I had m_cachedDetail:

The old version of CustomEvent cached the serialized form of detail; that’s what m_serializedDetail was about. So if we repeatedly used the detail in a world other than the one where the CustomEvent was created, the detail would be serialized only once, although it would be deserialized over and over again. Using m_cachedDetail I preserved a similar optimization, slightly different because it optimizes things even more if only two worlds are involved and does re-serialize when a third world is involved.

One other factor is whether the detail attribute needs to act like [SameObject]. I didn't fix that or think it through.

I wasn’t sure it was OK to remove the optimization the old code had to avoid serializing and deserializing over and over again, so I decided to put the cache in; also was similar to JSMessageEvent::data and I was trying to come up with a pattern I could reuse.

I honestly have no idea if this optimization is needed or not. I was trying not to change the performance characteristics too much in this patch.

Please let me know if any of that changes your mind about whether I should keep m_cachedDetail or get rid of it. Or what you think about the [SameObject] issue.
Comment 45 Chris Dumez 2018-02-03 14:57:50 PST 
(In reply to Darin Adler from comment #44)
> Comment on attachment 331305 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331305&action=review
> 
> >>>>> Source/WebCore/dom/CustomEvent.h:49
> >>>>> +    const JSValueInWrappedObject& detail() const { return m_detail; }
> >>>> 
> >>>> I do not quite understand why this class needs both a detail and a cachedDetail? What's the difference between the 2? Why cannot we rely solely on detail()? It seems cachedDetail is basically always initialized from detail.
> >>> 
> >>> m_cachedDetail can be m_detail cloned across worlds
> >> 
> >> But it sounds like an optimization, right? We could purely rely on m_detail and clone on demand when needed. I am not sure it is worth optimizing this. I do not even know how a CustomEvent could be passed across world.
> > 
> > The helper that handles the lifetime across worlds called cachedPropertyValue is used two places: JSCustomEvent::detail and JSMessageEvent::data. In the case of JSMessageEvent, I was preserving the existing caching that has reasons to be there besides the "cloned across worlds" issue. I guess you are saying that here in JSCustomEvent the caching isn’t needed or helpful. It’s easy for me to remove it, I suppose. I think I was hoping that the pattern would be more similar in all these classes. I can eliminate this caching.
> 
> OK, I figured more about why I had m_cachedDetail:
> 
> The old version of CustomEvent cached the serialized form of detail; that’s
> what m_serializedDetail was about. So if we repeatedly used the detail in a
> world other than the one where the CustomEvent was created, the detail would
> be serialized only once, although it would be deserialized over and over
> again. Using m_cachedDetail I preserved a similar optimization, slightly
> different because it optimizes things even more if only two worlds are
> involved and does re-serialize when a third world is involved.
> 
> One other factor is whether the detail attribute needs to act like
> [SameObject]. I didn't fix that or think it through.
> 
> I wasn’t sure it was OK to remove the optimization the old code had to avoid
> serializing and deserializing over and over again, so I decided to put the
> cache in; also was similar to JSMessageEvent::data and I was trying to come
> up with a pattern I could reuse.
> 
> I honestly have no idea if this optimization is needed or not. I was trying
> not to change the performance characteristics too much in this patch.
> 
> Please let me know if any of that changes your mind about whether I should
> keep m_cachedDetail or get rid of it. Or what you think about the
> [SameObject] issue.

Seems fine to keep cachedDetail for now to maintain existing behavior.

[SameObject] is only used for object or interface types, which is why the spec does not says [SameObject] for the detail attribute (which is of type 'any').

What the spec says is that the detail attribute must return the value it was initialized to. I do not believe we construct CustomEvents from inside the engine, they are always constructed by JS using a JSValue. We should always return that JSValue that was provided by JS when constructing the CustomEvent, as long as detail is accessed from the same world of course.

When detail is accessed from another world, I do not think the behavior is specified. In such case, we serialize the JSValue and deserialize it for use in another world. I personally wouldn't worry about returning the same value every time in this case.
Comment 46 Darin Adler 2018-02-07 20:14:42 PST 
(In reply to Chris Dumez from comment #45)
> When detail is accessed from another world, I do not think the behavior is
> specified. In such case, we serialize the JSValue and deserialize it for use
> in another world. I personally wouldn't worry about returning the same value
> every time in this case.

OK, I’ll go with that.

We might want to return to this and change that some day. I probably would want to return the same object every time. It seems strange to deserialize it every time it’s accessed rather than just once, and returning the same value every time seems to better match the behavior you get in the original world.
Comment 47 Darin Adler 2018-02-07 22:06:37 PST 
Committed r228260: <https://trac.webkit.org/changeset/228260>
Comment 48 Radar WebKit Bug Importer 2018-02-07 22:13:40 PST 
<rdar://problem/37341103>
Comment 49 Darin Adler 2018-02-08 09:16:23 PST 
I see error event tests failing here that did not fail when I tested before landing. Unfortunately I don’t have time to fix the problem right now, so if someone would either skip those tests or roll out my patch, that would be fine.

I should be able to resolve this soon once I have a little free time. But not this morning.
Comment 50 Chris Dumez 2018-02-08 09:39:40 PST 
(In reply to Darin Adler from comment #49)
> I see error event tests failing here that did not fail when I tested before
> landing. Unfortunately I don’t have time to fix the problem right now, so if
> someone would either skip those tests or roll out my patch, that would be
> fine.
> 
> I should be able to resolve this soon once I have a little free time. But
> not this morning.

I am looking into it. This change looks bad:
72	 	    JSDOMGlobalObject* globalObject = toJSDOMGlobalObject(&scriptExecutionContext, isolatedWorld());
 	74	    auto* globalObject = toJSDOMWindow(downcast<Document>(scriptExecutionContext).frame(), isolatedWorld());

What if the global object is a workerglobalscope and not a window? In JSErrorHandler.cpp.
Comment 51 Chris Dumez 2018-02-08 09:50:15 PST 
(In reply to Chris Dumez from comment #50)
> (In reply to Darin Adler from comment #49)
> > I see error event tests failing here that did not fail when I tested before
> > landing. Unfortunately I don’t have time to fix the problem right now, so if
> > someone would either skip those tests or roll out my patch, that would be
> > fine.
> > 
> > I should be able to resolve this soon once I have a little free time. But
> > not this morning.
> 
> I am looking into it. This change looks bad:
> 72	 	    JSDOMGlobalObject* globalObject =
> toJSDOMGlobalObject(&scriptExecutionContext, isolatedWorld());
>  	74	    auto* globalObject =
> toJSDOMWindow(downcast<Document>(scriptExecutionContext).frame(),
> isolatedWorld());
> 
> What if the global object is a workerglobalscope and not a window? In
> JSErrorHandler.cpp.

Follow-up fix in <https://trac.webkit.org/changeset/228276>. Please use commit queue for such big patches, especially if they contain last minutes changes like this one.

I'll monitor the bots to make sure we're back to green but locally, the workers WPT were passing again.
Comment 52 Antoine Quint 2018-02-09 00:10:45 PST 
My patch for https://bugs.webkit.org/show_bug.cgi?id=182608 stopped building after updating to ToT, which was caused by having jsNull() instead of JSC::jsNull() in JSCustomEvent::detail(). This will be fixed when that patch lands.
Comment 53 Ross Kirsling 2018-02-09 16:40:24 PST 
Follow-up patch related to the keyEvent -> underlyingPlatformEvent change:
https://bugs.webkit.org/show_bug.cgi?id=182663