Crash in blink::getTypeExtension

Crash in blink::getTypeExtension

BUG=666610
Review-Url: https://codereview.chromium.org/2530243002
Cr-Commit-Position: refs/heads/master@{#447807}
Committed: https://chromium.googlesource.com/chromium/src/+/91ce1ab1209aaec1750eb1c1c3b77d606333abe1

[Anton Obzhirov, 2016-11-26 01:36:32]

Description was changed from

==========
Crash in blink::getTypeExtension

BUG=666610
==========

to

==========
Crash in blink::getTypeExtension

BUG=666610
==========

[Anton Obzhirov, 2016-11-26 01:36:32]

a.obzhirov@samsung.com changed reviewers:
+ dominicc@chromium.org

[sof, 2016-11-26 07:36:31]

drive-by comments

https://codereview.chromium.org/2530243002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2530243002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:702: if (impl.hasIs())
Is |exceptionState| intentionally ignored here?

https://codereview.chromium.org/2530243002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:705: v8::Local<v8::String>
stringObject;
The spec says "Let 'is' be the value of |is| member of 'options', or null if no
such member exists", so the need for stringification of the dict is backward
compat motivated, presumably..? Is that a common use of createElement(), why
isn't it being use-counted?

A comment explaining why this is done, would be helpful for someone looking at
the code.

(V8StringResource<> is useful for doing toString() conversions, but I don't yet
fully understand the motivation for this case.)

[Anton Obzhirov, 2016-11-27 19:49:53]

https://codereview.chromium.org/2530243002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2530243002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:702: if (impl.hasIs())
On 2016/11/26 07:36:30, sof wrote:
> Is |exceptionState| intentionally ignored here?
Previously getTypeExtension was called in createElement/createElementNS with
unmodified state. Now it is not the case and it is better to do anyway so I will
add the check, thanks for spotting.

https://codereview.chromium.org/2530243002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:705: v8::Local<v8::String>
stringObject;
On 2016/11/26 07:36:30, sof wrote:
> The spec says "Let 'is' be the value of |is| member of 'options', or null if
no
> such member exists", so the need for stringification of the dict is backward
> compat motivated, presumably..? Is that a common use of createElement(), why
> isn't it being use-counted?
> 
> A comment explaining why this is done, would be helpful for someone looking at
> the code.
> 
> (V8StringResource<> is useful for doing toString() conversions, but I don't
yet
> fully understand the motivation for this case.)

Yes, stringification of the dictionary is for backward compatibility only.
Before introduction of the dictionary the second argument was converted to
string for all cases automatically. I think I missed use-counting here because
technically it wasn't a string argument but you are right, it has to be done
here as well.

For the code to be aligned with the spec the string argument should be
deprecated first but plz correct me if I am wrong.

[Anton Obzhirov, 2016-11-29 17:06:24]

Hi Dominic,

I started to make changes to update the patch and then I realized that since the
code uses stringOrOptions.isDictionary() in createElement as V1 flag essentially
there is no way for V0 to create custom element using stringified second
argument. It might break compatibility for V0 elements but it is probably very
rare case.

Should we remove stringification completely from getTypeExtension?

[dominicc (has gone to gerrit), 2016-12-07 07:50:45]

On 2016/11/29 at 17:06:24, a.obzhirov wrote:
> Hi Dominic,
> 
> I started to make changes to update the patch and then I realized that since
the code uses stringOrOptions.isDictionary() in createElement as V1 flag
essentially there is no way for V0 to create custom element using stringified
second argument. It might break compatibility for V0 elements but it is probably
very rare case.
> 
> Should we remove stringification completely from getTypeExtension?

Really sorry for the slow reply.

Could you clarify what will be broken? Stringified arguments, but not strings,
for V0? That's probably fine--it sounds like an edge case of an edge case.

Let me reiterate the goals we have here, it might help:

- We want to remove stringifying the second argument, including taking an actual
string parameter.
- We want content to move from document.registerElement to
customElements.define.
- In general, we don't want to break content or churn APIs unnecessarily.

[Anton Obzhirov, 2016-12-09 11:35:39]

On 2016/12/07 07:50:45, dominicc wrote:
> On 2016/11/29 at 17:06:24, a.obzhirov wrote:
> > Hi Dominic,
> > 
> > I started to make changes to update the patch and then I realized that since
> the code uses stringOrOptions.isDictionary() in createElement as V1 flag
> essentially there is no way for V0 to create custom element using stringified
> second argument. It might break compatibility for V0 elements but it is
probably
> very rare case.
> > 
> > Should we remove stringification completely from getTypeExtension?
> 
> Really sorry for the slow reply.
NP, was quite busy recently.
> 
> Could you clarify what will be broken? Stringified arguments, but not strings,
> for V0? That's probably fine--it sounds like an edge case of an edge case.
> 
> Let me reiterate the goals we have here, it might help:
> 
> - We want to remove stringifying the second argument, including taking an
actual
> string parameter.
> - We want content to move from document.registerElement to
> customElements.define.
> - In general, we don't want to break content or churn APIs unnecessarily.

Yes, I meant stringified second argument for V0.
Makes sense, I will remove the stringification and update the patch today.

[Anton Obzhirov, 2016-12-09 16:36:59]

Updated, plz have a look.

[dominicc (has gone to gerrit), 2017-01-19 05:34:46]

Comments inline.

https://codereview.chromium.org/2530243002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/custom/crash-in-getTypeExtension.html
(right):

https://codereview.chromium.org/2530243002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/custom/crash-in-getTypeExtension.html:1:
<!DOCTYPE html>
Could you change the test in this file?

- Register a v0 custom element which extends div and is called x-foo.
- Add a TODO to switch to customElements.define when customized built-in
elements are supported.
- Add a test where toString returns x-foo. Assert that x-foo is NOT created,
just an ordinary DIV is. Assert that toString is *not* called. (It's not called,
right?)

https://codereview.chromium.org/2530243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2530243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:698: // Do not handle
stringification, but count it as a string handling
This changes behavior because we won't toString the argument now. I would not
use count this path with this counter, because this path is already changed. If
you want to count it at all, I would add a second counter.

[Anton Obzhirov, 2017-01-19 16:45:44]

https://codereview.chromium.org/2530243002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/custom/crash-in-getTypeExtension.html
(right):

https://codereview.chromium.org/2530243002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/custom/crash-in-getTypeExtension.html:1:
<!DOCTYPE html>
On 2017/01/19 05:34:46, dominicc wrote:
> Could you change the test in this file?
> 
> - Register a v0 custom element which extends div and is called x-foo.
> - Add a TODO to switch to customElements.define when customized built-in
> elements are supported.
> - Add a test where toString returns x-foo. Assert that x-foo is NOT created,
> just an ordinary DIV is. Assert that toString is *not* called. (It's not
called,
> right?)

Yes, with this change it will not be called at all. Will do.

https://codereview.chromium.org/2530243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2530243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:698: // Do not handle
stringification, but count it as a string handling
On 2017/01/19 05:34:46, dominicc wrote:
> This changes behavior because we won't toString the argument now. I would not
> use count this path with this counter, because this path is already changed.
If
> you want to count it at all, I would add a second counter.
I think no need to count this case it should be very rare.

[Anton Obzhirov, 2017-01-21 21:32:42]

I had also to change the test after I realized we get now NotFoundError if
create element with invalid dictionary. Plz have a look.

[dominicc (has gone to gerrit), 2017-01-31 22:41:38]

lgtm

https://codereview.chromium.org/2530243002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/custom/crash-in-getTypeExtension.html
(right):

https://codereview.chromium.org/2530243002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/custom/crash-in-getTypeExtension.html:12:
assert_false(toStringCalled, "toString should not have been called.");
Use single quotes for consistency here and on line 13.

[dominicc (has gone to gerrit), 2017-01-31 22:42:57]

Minor nit inline.

[Anton Obzhirov, 2017-02-02 16:38:05]

The CQ bit was checked by a.obzhirov@samsung.com

[Anton Obzhirov, 2017-02-02 16:38:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominicc@chromium.org
Link to the patchset: https://codereview.chromium.org/2530243002/#ps60001
(title: "Crash in blink::getTypeExtension")

[commit-bot: I haz the power, 2017-02-02 16:38:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 18:48:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486053485761690,
"parent_rev": "0b2a5078b32cbed2cf7465073315d90356b3aa6c", "commit_rev":
"91ce1ab1209aaec1750eb1c1c3b77d606333abe1"}

[commit-bot: I haz the power, 2017-02-02 18:49:19]

Description was changed from

==========
Crash in blink::getTypeExtension

BUG=666610
==========

to

==========
Crash in blink::getTypeExtension

BUG=666610

Review-Url: https://codereview.chromium.org/2530243002
Cr-Commit-Position: refs/heads/master@{#447807}
Committed:
https://chromium.googlesource.com/chromium/src/+/91ce1ab1209aaec1750eb1c1c3b7...
==========

[commit-bot: I haz the power, 2017-02-02 18:49:23]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/91ce1ab1209aaec1750eb1c1c3b7...