Bug 87454 - createContextualFragment and insertAdjacentHTML should throw syntax error
: createContextualFragment and insertAdjacentHTML should throw syntax error
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Ryosuke Niwa
:
Depends on: 87339
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-24 20:25 PDT by Ryosuke Niwa
Modified: 2012-05-25 15:37 PDT (History)
9 users (show)

See Also:


Attachments
Fixes the bug (12.59 KB, patch)
2012-05-24 20:52 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed Qt build (17.13 KB, patch)
2012-05-24 20:59 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (527.62 KB, application/zip)
2012-05-24 22:51 PDT, WebKit Review Bot
no flags Details
Fixed tests (19.73 KB, patch)
2012-05-25 00:34 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Address comments while waiting for a review (20.21 KB, patch)
2012-05-25 13:54 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-05-24 20:25:25 PDT
Right now, createContextualFragment throws NOT_SUPPORTED_ERR and insertAdjacentHTML doesn't throw any errors.

We should throw SYNTAX_ERR to be consistent with the spec and Firefox.
http://html5.org/specs/dom-parsing.html#parsing
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-xhtml-syntax.html#xml-fragment-parsing-algorithm
Comment 1 Ryosuke Niwa 2012-05-24 20:52:31 PDT
Created attachment 143960 [details]
Fixes the bug
Comment 2 Ryosuke Niwa 2012-05-24 20:59:44 PDT
Created attachment 143962 [details]
Fixed Qt build
Comment 3 Ryosuke Niwa 2012-05-24 21:05:14 PDT
Comment on attachment 143962 [details]
Fixed Qt build

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

> Source/WebCore/editing/markup.h:56
> -    PassRefPtr<DocumentFragment> createContextualFragment(const String&, Element*,  FragmentScriptingPermission);
> +    PassRefPtr<DocumentFragment> createContextualFragment(const String&, Element*, FragmentScriptingPermission, ExceptionCode&);

Oops, I meant to change Element* to HTMLElement*. Will do once I get a review.
Comment 4 WebKit Review Bot 2012-05-24 22:51:38 PDT
Comment on attachment 143962 [details]
Fixed Qt build

Attachment 143962 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12800415

New failing tests:
fast/parser/xhtml-innerhtml-null-byte.xhtml
fast/innerHTML/innerHTML-changing-document-properties.xhtml
fast/parser/xhtml-innerhtml-null-byte-first.xhtml
Comment 5 WebKit Review Bot 2012-05-24 22:51:49 PDT
Created attachment 143981 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 6 Ryosuke Niwa 2012-05-25 00:34:37 PDT
Created attachment 143992 [details]
Fixed tests
Comment 7 Alexey Proskuryakov 2012-05-25 10:59:39 PDT
Comment on attachment 143992 [details]
Fixed tests

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

Does IE support insertAdjacentHTML? Does it raise exceptions?

> Source/WebCore/editing/markup.cpp:669
> +    ExceptionCode ignoredEC;
> +    RefPtr<DocumentFragment> fragment = createContextualFragment(markup, fakeBody.get(), scriptingPermission, ignoredEC);

Seems worth commenting why it's ignored.

> Source/WebKit/qt/Api/qwebelement.cpp:1063
> +    RefPtr<DocumentFragment> fragment =  createContextualFragment(markup, toHTMLElement(m_element), AllowScriptingContent, exception);
>  
>      if (m_element->hasChildNodes())
>          m_element->insertBefore(fragment, m_element->firstChild(), exception);

What happens if this raises an exception? Looks like a crash due to null fragment to me.

> Source/WebKit/qt/Api/qwebelement.cpp:1113
>      ExceptionCode exception = 0;
> +    RefPtr<DocumentFragment> fragment = createContextualFragment(markup, toHTMLElement(parent), AllowScriptingContent, exception);
> +
>      parent->insertBefore(fragment, m_element, exception);

Ditto.

> Source/WebKit/qt/Api/qwebelement.cpp:1164
> +    RefPtr<DocumentFragment> fragment = createContextualFragment(markup, toHTMLElement(parent), AllowScriptingContent, exception);
> +
>      if (!m_element->nextSibling())
>          parent->appendChild(fragment, exception);

Ditto.
Comment 8 Ryosuke Niwa 2012-05-25 11:11:52 PDT
(In reply to comment #7)
> (From update of attachment 143992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143992&action=review
> 
> Does IE support insertAdjacentHTML? Does it raise exceptions?

IE doesn't throw exceptions for all cases tested here. I'm making insertAdjacentHTML throw for consistency with other APIs. (e.g. it's surprising if innerHTML throws and insertAdjacentHTML doesn't throw for the same markup). Given that insertAdjacentHTML isn't a particularly popular API and that this only happens in XHTML document, I don't expect there to be any compat. issues.

> > Source/WebCore/editing/markup.cpp:669
> > +    ExceptionCode ignoredEC;
> > +    RefPtr<DocumentFragment> fragment = createContextualFragment(markup, fakeBody.get(), scriptingPermission, ignoredEC);
> 
> Seems worth commenting why it's ignored.

createFragmentFromMarkup is only used for copy & paste purposes (and moving paragraphs). I'm intending to rename this function to make it clear why ignoring the exception makes sense. But do you have a suggestion on what kind of comment we should add?

> > Source/WebKit/qt/Api/qwebelement.cpp:1063
> > +    RefPtr<DocumentFragment> fragment =  createContextualFragment(markup, toHTMLElement(m_element), AllowScriptingContent, exception);
> >  
> >      if (m_element->hasChildNodes())
> >          m_element->insertBefore(fragment, m_element->firstChild(), exception);
> 
> What happens if this raises an exception? Looks like a crash due to null fragment to me.

Yeah, but I'm not changing the behavior here. We have always returned null fragments when XML parser returned null. Folks from the Qt port needs to address this issue.
Comment 9 Alexey Proskuryakov 2012-05-25 11:27:26 PDT
> insertAdjacentHTML isn't a particularly popular API and that this only happens in XHTML document

It does seem that the general rule of sticking to majority behavior is weaker here, and and consistency arguments feel a bit stronger than usual.
Comment 10 Eric Seidel 2012-05-25 12:33:14 PDT
Comment on attachment 143992 [details]
Fixed tests

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

> Source/WebCore/ChangeLog:8
> +        Fixed the bug and reduced the code duplication.

Adding an exception is always scary.  Can you document here what other browsers do, and why we're making this change?  Is there a sepc which says we should throw here?
Comment 11 Ryosuke Niwa 2012-05-25 13:29:19 PDT
(In reply to comment #10)
> (From update of attachment 143992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143992&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Fixed the bug and reduced the code duplication.
> 
> Adding an exception is always scary.  Can you document here what other browsers do, and why we're making this change?  Is there a sepc which says we should throw here?

Yes, see my original bug report:

Right now, createContextualFragment throws NOT_SUPPORTED_ERR and insertAdjacentHTML doesn't throw any errors.

We should throw SYNTAX_ERR to be consistent with the spec and Firefox.
http://html5.org/specs/dom-parsing.html#parsing
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-xhtml-syntax.html#xml-fragment-parsing-algorithm

I can include that in the change log if you want.
Comment 12 Ryosuke Niwa 2012-05-25 13:54:58 PDT
Created attachment 144143 [details]
Address comments while waiting for a review
Comment 13 Darin Adler 2012-05-25 13:58:57 PDT
Comment on attachment 144143 [details]
Address comments while waiting for a review

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

> Source/WebCore/editing/markup.cpp:1055
> +PassRefPtr<DocumentFragment> createContextualFragment(const String& markup, HTMLElement* htmlElement, FragmentScriptingPermission scriptingPermission, ExceptionCode& ec)

I don’t think we should rename the element argument even though we are changing the type.
Comment 14 Ryosuke Niwa 2012-05-25 14:00:47 PDT
Comment on attachment 144143 [details]
Address comments while waiting for a review

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

>> Source/WebCore/editing/markup.cpp:1055
>> +PassRefPtr<DocumentFragment> createContextualFragment(const String& markup, HTMLElement* htmlElement, FragmentScriptingPermission scriptingPermission, ExceptionCode& ec)
> 
> I don’t think we should rename the element argument even though we are changing the type.

Okay. It'll be a bit invasive but no problem.
Comment 15 Ryosuke Niwa 2012-05-25 15:02:37 PDT
Committed r118570: <http://trac.webkit.org/changeset/118570>