Bug 162996

Summary:

Upgrading and constructing element should always report exception instead of rethrowing

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

DOM

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, commit-queue, dbates, esprehn+autocc, gyuyoung.kim, kangil.han, koivisto, mkwst, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari Technology Preview

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

154907

Attachments:

Description	Flags
Fixes the bug	none
Patch for landing	none

Ryosuke Niwa

Reported 2016-10-05 22:24:17 PDT

When upgrading or constructing an element synchronously inside the HTML parser and document.createElement, we must report the exception instead of rethrowing the exception.

Attachments
Fixes the bug (46.67 KB, patch) 2016-10-05 22:40 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch for landing (46.65 KB, patch) 2016-10-06 17:15 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2016-10-05 22:40:03 PDT

Created attachment 290782 [details] Fixes the bug

Radar WebKit Bug Importer

Comment 2 2016-10-06 00:07:55 PDT

<rdar://problem/28647657>

Darin Adler

Comment 3 2016-10-06 12:42:04 PDT

Comment on attachment 290782 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=290782&action=review > Source/WebCore/bindings/js/JSCustomElementInterface.cpp:72 > + return element.get(); The get() here means we get reference count churn. Can we compile a version that transfers ownership instead? Maybe WTFMove(element) works? If not, then we should fix the Ref class so it does. > Source/WebCore/bindings/js/JSCustomElementInterface.cpp:91 > + RefPtr<Element> element = constructCustomElementSynchronously(document, vm, state, m_constructor.get(), localName); Can we use auto here instead of RefPtr? > Source/WebCore/html/parser/HTMLDocumentParser.cpp:200 > + Ref<Element> newElement = elementInterface.constructElementWithFallback(*document(), constructionData->name); Can we use auto here?

Ryosuke Niwa

Comment 4 2016-10-06 13:16:27 PDT

Thanks for the review. (In reply to comment #3) > Comment on attachment 290782 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=290782&action=review > > > Source/WebCore/bindings/js/JSCustomElementInterface.cpp:72 > > + return element.get(); > > The get() here means we get reference count churn. Can we compile a version > that transfers ownership instead? Maybe WTFMove(element) works? If not, then > we should fix the Ref class so it does. WTFMove(element) fails because Ref<HTMLUnknownElement>&& can be adopted by Ref<Element>&& :( I agree we should fix that, not in this patch though. > > > Source/WebCore/bindings/js/JSCustomElementInterface.cpp:91 > > + RefPtr<Element> element = constructCustomElementSynchronously(document, vm, state, m_constructor.get(), localName); > > Can we use auto here instead of RefPtr? Sure. > > Source/WebCore/html/parser/HTMLDocumentParser.cpp:200 > > + Ref<Element> newElement = elementInterface.constructElementWithFallback(*document(), constructionData->name); > > Can we use auto here? Sure.

Ryosuke Niwa

Comment 5 2016-10-06 17:15:06 PDT

Created attachment 290872 [details] Patch for landing

WebKit Commit Bot

Comment 6 2016-10-06 17:49:01 PDT

Comment on attachment 290872 [details] Patch for landing Clearing flags on attachment: 290872 Committed r206890: <http://trac.webkit.org/changeset/206890>

WebKit Commit Bot

Comment 7 2016-10-06 17:49:06 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.