1294100 - TypeError: Argument 2 of Document.createElement can't be converted to a dictionary.

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Pouyan]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160809004002

Steps to reproduce:

Open a page that contains the following JavaScript line:
grid.loadingMsg = document.createElement("div", "idLoadingSpinner");


Actual results:

"""The TypeError: Argument 2 of Document.createElement can't be converted to a dictionary""" error will be delivered in the console and JavaScript stops to work.


Expected results:

Showing the page. The same page was tested with chrome Version 52.0.2743.116 (64-bit) and no problem was found.

This could also help:
http://stackoverflow.com/questions/30767350/document-createelement-multiple-arguments

Comment 1

[Tooru Fujisawa [:arai]]

The 2nd parameter of `Document.createDocument` was added by bug 856140 and changed by bug 1276579, to follow the spec change.

https://dom.spec.whatwg.org/#dfnReturnLink-0
> [NewObject] Element createElement(DOMString localName, optional ElementCreationOptions options);

https://dom.spec.whatwg.org/#dom-document-createelement
> The createElement(localName, options) method, when invoked, must run these steps:
> ...
> 3. Let `is` be the value of `is` member of `options`, or `null` if no such member exists.


The type of the 2nd parameter is ElementCreationOptions, that is a dictionary:

https://dom.spec.whatwg.org/#dictdef-elementcreationoptions
> dictionary ElementCreationOptions
> {
>   DOMString is;
> };


Then passing non-object value to dictionary-typed parameter should throw:

https://www.w3.org/TR/WebIDL/#es-dictionary
> 1. If Type(V) is not Object, then throw a TypeError.


then, Google Chrome doesn't follow the latest spec, but following previous spec, that the type of 2nd parameter was string.
Here's input and output in console:
  > document.createElement("a", "b")
  <a is=​"b">​</a>​
  > document.createElement("a", {is:"bc"})
  <a is=​"[object Object]​">​</a>​


So, this sounds like a spec issue for web backward compatibility.


smaug, this is a similar issue as bug 1244480.
How do you think about this case?

Comment 2

[Anne (:annevk)]

Tooru, how many cases have we hit thus far?

Pouyan, is this something you can fix?

Comment 3

[Tooru Fujisawa [:arai]]

only in the examples in Polymer documentation, that is linked from the stackoverflow page in comment #0 so far
  https://www.polymer-project.org/1.0/docs/devguide/registering-elements

So this could be the specific issue to Polymer.
not sure how widely it's used tho...

Comment 4

[Anne (:annevk)]

I filed https://github.com/Polymer/docs/issues/1705. Mike, you might want to poke them too if you have a channel.

Comment 5

[Pouyan]

Anne, it is used in the propriety software we use at work => Groupwise WebAccess, I will try to contact the support to see if that can be changed.

Comment 6

[Anne (:annevk)]

Pouyan, thank you, appreciated. Though it being in a product suggests we likely have to make this work... Sigh.

Comment 7

[Anne (:annevk)]

I also filed https://github.com/w3c/webcomponents/issues/544 so that we can consider a change to the standard.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

[Tracking Requested - why for this release]:
web compat


We need to probably add some dummy
DOMString or ElementCreationOptions options
and do nothing when DOMString is passed

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi John, is this something you could help?

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi John, any update here?

Comment 11

[John Dai[:jdai]]

Attachment: wip

Yes, after apply my patch, there are 4 kinds of errors as the following:

1. 'nsIDocument::DeprecatedOperations' has not been declared in BindingUtils.h

 0:07.25 In file included from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/DocumentBinding.h:10:0,
 0:07.25                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsIDocument.h:27,
 0:07.25                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/dom/xslt/xpath/txXPathNode.h:11,
 0:07.25                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/dom/xslt/xpath/txXPathTreeWalker.h:10,
 0:07.26                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/dom/xslt/xpath/txNameTest.cpp:9,
 0:07.26                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dom/xslt/xpath/Unified_cpp_dom_xslt_xpath1.cpp:2:
 0:07.26 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/BindingUtils.h:3345:33: error: 'nsIDocument::DeprecatedOperations' has not been declared

2. invalid application of 'sizeof' to incomplete type 'mozilla::dom::binding_detail::FakeString'

 0:08.90 In file included from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ArrayUtils.h:21:0,
 0:08.90                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/PodOperations.h:19,
 0:08.90                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/jspubtd.h:17,
 0:08.90                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsCycleCollectionTraversalCallback.h:10,
 0:08.90                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsCycleCollectionNoteChild.h:13,
 0:08.90                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:33,
 0:08.90                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/DOMEventTargetHelper.h:10,
 0:08.90                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/CellBroadcast.h:11,
 0:08.90                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/dom/cellbroadcast/CellBroadcast.cpp:7,
 0:08.90                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dom/cellbroadcast/Unified_cpp_dom_cellbroadcast0.cpp:2:
 0:08.91 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Alignment.h: In instantiation of 'union mozilla::AlignedStorage2<mozilla::dom::binding_detail::FakeString>::U':
 0:08.91 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Alignment.h:139:5:   required from 'struct mozilla::AlignedStorage2<mozilla::dom::binding_detail::FakeString>'
 0:08.91 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/UnionMember.h:22:22:   required from 'class mozilla::dom::UnionMember<mozilla::dom::binding_detail::FakeString>'
 0:08.91 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/DocumentBinding.h:109:46:   required from here
 0:08.91 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Alignment.h:137:23: error: invalid application of 'sizeof' to incomplete type 'mozilla::dom::binding_detail::FakeString'

3. const union mozilla::AlignedStorage2<mozilla::dom::binding_detail::FakeString>::U' has no member named 'mBytes'

 0:08.91 In file included from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ArrayUtils.h:21:0,
 0:08.91                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/PodOperations.h:19,
 0:08.91                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/jspubtd.h:17,
 0:08.91                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsCycleCollectionTraversalCallback.h:10,
 0:08.91                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsCycleCollectionNoteChild.h:13,
 0:08.91                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:33,
 0:08.91                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/DOMEventTargetHelper.h:10,
 0:08.91                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/CellBroadcast.h:11,
 0:08.91                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/dom/cellbroadcast/CellBroadcast.cpp:7,
 0:08.91                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dom/cellbroadcast/Unified_cpp_dom_cellbroadcast0.cpp:2:
 0:08.91 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Alignment.h: In instantiation of 'T* mozilla::AlignedStorage2<T>::addr() [with T = mozilla::dom::binding_detail::FakeString]':
 0:08.91 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/UnionMember.h:44:12:   required from 'T& mozilla::dom::UnionMember<T>::Value() [with T = mozilla::dom::binding_detail::FakeString]'
 0:08.91 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/DocumentBinding.h:175:35:   required from here
 0:08.91 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Alignment.h:142:66: error: 'union mozilla::AlignedStorage2<mozilla::dom::binding_detail::FakeString>::U' has no member named 'mBytes'
 0:08.91 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Alignment.h: In instantiation of 'const T* mozilla::AlignedStorage2<T>::addr() const [with T = mozilla::dom::binding_detail::FakeString]':
 0:08.91 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/UnionMember.h:48:12:   required from 'const T& mozilla::dom::UnionMember<T>::Value() const [with T = mozilla::dom::binding_detail::FakeString]'
 0:08.91 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/DocumentBinding.h:210:33:   required from here
 0:08.91 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Alignment.h:141:69: error: 'const union mozilla::AlignedStorage2<mozilla::dom::binding_detail::FakeString>::U' has no member named 'mBytes'

4. invalid initialization of reference of type 'const nsAString_internal&' from expression of type 'const mozilla::dom::binding_detail::FakeString'

 0:09.05 In file included from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/nsIDocument.h:27:0,
 0:09.05                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/BindingUtils.h:33,
 0:09.05                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/ToJSValue.h:12,
 0:09.05                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/EventListenerBinding.h:12,
 0:09.05                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/EventListenerManager.h:11,
 0:09.05                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/DOMEventTargetHelper.h:19,
 0:09.06                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/dom/battery/BatteryManager.h:11,
 0:09.06                  from /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/dom/battery/BatteryManager.cpp:9:
 0:09.06 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/DocumentBinding.h: In member function 'const nsAString_internal& mozilla::dom::ElementCreationOptionsOrString::GetAsString() const':
 0:09.06 /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/DocumentBinding.h:210:33: error: invalid initialization of reference of type 'const nsAString_internal&' from expression of type 'const mozilla::dom::binding_detail::FakeString'

Comment 12

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #11)
> Created attachment 8781822 [details] [diff] [review]
> wip
> 
> Yes, after apply my patch, there are 4 kinds of errors as the following:
> 
> 1. 'nsIDocument::DeprecatedOperations' has not been declared in
> BindingUtils.h
> 
>  0:07.25 In file included from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/mozilla/dom/DocumentBinding.h:10:0,
>  0:07.25                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/nsIDocument.h:27,
>  0:07.25                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/dom/
> xslt/xpath/txXPathNode.h:11,
>  0:07.25                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/dom/
> xslt/xpath/txXPathTreeWalker.h:10,
>  0:07.26                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/dom/
> xslt/xpath/txNameTest.cpp:9,
>  0:07.26                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dom/xslt/xpath/Unified_cpp_dom_xslt_xpath1.cpp:2:
>  0:07.26
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/mozilla/dom/BindingUtils.h:3345:33: error:
> 'nsIDocument::DeprecatedOperations' has not been declared
> 

For this error, I found there has a Circular Dependencies in the header files, I can add forward declaration either in BindingUtil.h or DocumentBinding.h.
If I add forward declaration in DocumentBinding.h, it will hit issue 2, I don't know about concrete 'sizeof' to incomplete type 'mozilla::dom::binding_detail::FakeString'.
On the other hand, if I add forward declaration in BindingUtil.h, I needs to modify a lot of files because nsIDocument::DeprecatedOperations is a enum in class. Therefore, I need to pull nsIDocument::DeprecatedOperations enum out of class and move into a namespace.


> 2. invalid application of 'sizeof' to incomplete type
> 'mozilla::dom::binding_detail::FakeString'
> 
>  0:08.90 In file included from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/mozilla/ArrayUtils.h:21:0,
>  0:08.90                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/mozilla/PodOperations.h:19,
>  0:08.90                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/jspubtd.h:17,
>  0:08.90                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/nsCycleCollectionTraversalCallback.h:10,
>  0:08.90                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/nsCycleCollectionNoteChild.h:13,
>  0:08.90                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:33,
>  0:08.90                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/mozilla/DOMEventTargetHelper.h:10,
>  0:08.90                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/mozilla/dom/CellBroadcast.h:11,
>  0:08.90                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/dom/
> cellbroadcast/CellBroadcast.cpp:7,
>  0:08.90                  from
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dom/cellbroadcast/Unified_cpp_dom_cellbroadcast0.cpp:2:
>  0:08.91
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/mozilla/Alignment.h: In instantiation of
> 'union
> mozilla::AlignedStorage2<mozilla::dom::binding_detail::FakeString>::U':
>  0:08.91
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/mozilla/Alignment.h:139:5:   required from
> 'struct mozilla::AlignedStorage2<mozilla::dom::binding_detail::FakeString>'
>  0:08.91
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/mozilla/dom/UnionMember.h:22:22:   required
> from 'class
> mozilla::dom::UnionMember<mozilla::dom::binding_detail::FakeString>'
>  0:08.91
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/mozilla/dom/DocumentBinding.h:109:46:  
> required from here
>  0:08.91
> /media/john/c6200233-c3c3-4f24-8744-a964b0584bfa/john/workspace/firefox/obj-
> x86_64-pc-linux-gnu/dist/include/mozilla/Alignment.h:137:23: error: invalid
> application of 'sizeof' to incomplete type
> 'mozilla::dom::binding_detail::FakeString'
>

Comment 13

[John Dai[:jdai]]

Attachment: Part 1: wip

Finally, I added forward declaration in BindingUtil.h, and moved nsIDocument::DeprecatedOperations enum to mozilla::dom namespace. 
Also, do nothing when DOMString is passed in nsDocument::CreateElement().
Hi Olli, 
May I have your early feedback for this changes? Thank you.

Comment 14

[John Dai[:jdai]]

Attachment: Part 2: wip

This patch mainly fix comment #11 errors.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8781957 [details] [diff] [review]
Part 2: wip

This has all sorts of random whitespace changes. Could you please remove them
(or even better, have separate patch which fixes just whitespace handling :)).

And seemingly random change to DOMEventTargetHelper.h

+//#include "nsIDocument.h"?

In many cases you shouldn't need to use mozilla::dom. Say, Attr.cpp code is already in mozilla::dom namespace.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8781956 [details] [diff] [review]
Part 1: wip

> static UseCounter
>-OperationToUseCounter(nsIDocument::DeprecatedOperations aOperation)
>+OperationToUseCounter(mozilla::dom::DeprecatedOperations aOperation)
do you need to use mozilla::dom here?

Comment 17

[Mike Taylor [:miketaylr]]

(sorry, was on PTO. clearing ni? because it seems the scope here is beyond tech evangelism)

Comment 18

[John Dai[:jdai]]

Still has Mac, Windows, and Android build errors need to fix.[1]

[1]https://treeherder.mozilla.org/#/jobs?repo=try&revision=9de56dce10ef132b82dcd87c3a88bbf23766b881&filter-tier=1

Comment 19

[Boris Zbarsky [:bzbarsky]]

We do NOT want to include BindingUtils.h in nsIDocument.h.  That header pulls in all sorts of junk, and nsIDocument is included in a _lot_ of places.  The include cycles you ran into are only part of the problem; the other bits include much slower compilation times and much greater danger of future cycles.

Why do we need to include BindingUtils.h from nsIDocument.h, exactly?

Comment 20

[John Dai[:jdai]]

After I added union type at createElement() in Document.webidl, codegen generated DocumentBinding.h which included BindingUtils.h. Hence, there is a circular dependencies in the header files which are as following,

1) DocumentBinding.h include BindingUtils.h, because it needs to use binding_detail::FakeString and binding_detail::FastElementCreationOptions.
2) BindingUtils.h include nsIDocument.h, because it needs to use nsIDocument::DeprecatedOperations enum. 
3) nsIDocument.h include DocumentBinding.h, becauese it needs to use mozilla::dom::VisibilityState::Visible which is declared in DocumentBinding.h.

In my patch, I used forward declaration to broke down nsIDocument, since nsIDocument::DeprecatedOperations is class enum in nsIDocument.h. Therefore, I moved nsIDocument::DeprecatedOperations enum to mozilla::dom namespace.

Hi Bz, 
Do you have better suggestion? If there is anything I did't explain it well, please let me know.

Comment 21

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 1: Remove redundant trailing spaces.

Comment 22

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 2: Support DOMString as Argument 2 of Document.createElement.

Comment 23

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 3: Move DeprecatedOperations enum to mozilla::dom.

Comment 24

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 4: Modify Codegen to generate enum DeprecatedOperations namespace binding files.

Comment 25

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 5: Add missing include header file and modify relative DeprecatedOperations namespace.

Comment 26

[John Dai[:jdai]]

Those patches aren't ready to review, it only can pass build on linux.

Comment 27

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] from comment #19)
> We do NOT want to include BindingUtils.h in nsIDocument.h.  That header
> pulls in all sorts of junk, and nsIDocument is included in a _lot_ of
> places.  The include cycles you ran into are only part of the problem; the
> other bits include much slower compilation times and much greater danger of
> future cycles.
> 
> Why do we need to include BindingUtils.h from nsIDocument.h, exactly?

No, we don't need to include BindingUtils.h from nsIDocument.h. Comment #13 and Comment #14 are draft patches with a lot of redundant include. I uploaded new patches and cleaned up a lot of junk including files. I think it can give you more clear about what I done before.

Comment 28

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #27)
> (In reply to Boris Zbarsky [:bz] from comment #19)
> > We do NOT want to include BindingUtils.h in nsIDocument.h.  That header
> > pulls in all sorts of junk, and nsIDocument is included in a _lot_ of
> > places.  The include cycles you ran into are only part of the problem; the
> > other bits include much slower compilation times and much greater danger of
> > future cycles.
> > 
> > Why do we need to include BindingUtils.h from nsIDocument.h, exactly?
> 
> No, we don't need to include BindingUtils.h from nsIDocument.h. Comment #13
> and Comment #14 are draft patches with a lot of redundant include. I
> uploaded new patches and cleaned up a lot of junk including files. I think
> it can give you more clear about what I done before.

One thing I forgot to mention, even we don't include BindingUtils.h from nsIDocument.h. BindingUtils.h is still included in DocumentBinding.h.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> codegen generated DocumentBinding.h which included BindingUtils.h

Ah, that's what's broken.  We went to great pains to never include BindingUtils.h from *Binding.h files, precisely because it causes problems like this, but it looks like that got broken with the move to per-binding-header unions.

If we only include BindingUtils.h for FakeString, we should probably move FakeString into a separate header and only include that header in both UnionTypes (in "includes"; "implincludes" should still get BindingUtils.h) and in the CGBindingRoot bindingDeclareHeaders bits.  The latter should solve your problem, I would expect.

Comment 30

[John Dai[:jdai]]

Attachment: Bug 1294100 - Support DOMString as Argument 2 of Document.createElement.

I used WebIDL overloads to solve this problem cleanly without modifying a lot of header files.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c562f491da97c8b612902a1ac76b1d93aa3b410d&filter-tier=1

Comment 31

[Boris Zbarsky [:bzbarsky]]

> I used WebIDL overloads to solve this problem cleanly without modifying a lot of header files.

Please make sure a bug is filed on the union breakage.  That still needs to be fixed, even if you have worked around it by not using unions here.

Comment 32

[John Dai[:jdai]]

Comment on attachment 8784325 [details] [diff] [review]
Bug 1294100 - Support DOMString as Argument 2 of Document.createElement.

Let me do it in a right way.

Comment 33

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 1: Support DOMString as Argument 2 of Document.createElement.

Comment 34

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 2: Modify Codegen to generate including FakeString's binding files and fix build errors.

In this patch, I moved FakeString into a separate header file to break down circular dependencies as comment #29 suggested. Also, I modified codegen code and added missing include headers to prevent building errors. In order to fix building errors from Mac, I added include guards in nsIDocument.h. Because FakeString needs MOZILLA_INTERNAL_API, but Mac use nsStringAPI.h which is a non-MOZILLA_INTERNAL_API.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0b6855f75c4415405bc53f854660cf8a6fbf60b

Comment 35

[Marcia Knous [:marcia]]

Tracking 51+ for web compat.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8785903 [details] [diff] [review]
Bug 1294100 - Part 1: Support DOMString as Argument 2 of Document.createElement.

This needs to actually be part 2, since it doesn't build without the fakestring/codegen changes, right?

>+  ElementCreationOptionsOrString options;
>   nsCOMPtr<Element> element = CreateElement(aTagName, options, rv);

This is not OK: it's passing in an uninitialized union, which bindings guarantee to _never_ do.  You need to init it to one or the other possible value here.

>+    is = CheckCustomElementName(aOptions.GetAsElementCreationOptions(),

This is preexisting, but CheckCustomElementName const_casts the string which is very much NOT ok to do with strings that come from bindings.  I have no idea why it's doing that, but it needs to stop.  The return type from CheckCustomElementName should be "const nsString*" and the const_cast needs to go away.

>+  ElementCreationOptionsOrString options;
>   RefPtr<Element> element = doc->CreateElement(VIDEO_TAG, options, error);

Again, please don't pass in uninitialized unions.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8785904 [details] [diff] [review]
Bug 1294100 - Part 2: Modify Codegen to generate including FakeString's binding files and fix build errors.

>+#ifdef MOZILLA_INTERNAL_API
>   // Our visibility state
>   mozilla::dom::VisibilityState mVisibilityState;
>+#endif

This is a huge footgun.  If there are any inline methods that touch later members, they will now do the wrong thing when !MOZILLA_INTERNAL_API.

We need to figure out some way of solving this problem... maybe in the #else branch declare it with whatever size type VisibilityState has and add static asserts to ensure the two never get out of sync?

>+        bindingDeclareHeaders["mozilla/dom/BindingUtils.h"] = any('Object' in t.name for t in unionStructs)

Please don't rely on the name bit like that.  For example, if an interface is named SillyObject, this code will trigger.

You should be checking the types in the flatMemberTypes of the union structs involved here.

>+        # Need BindingUtils.h for CallerSubsumes

This is in the UnionTypes bit, right?  Please document here that this is only needed for SetToObject.  And in SetToObject's definition document that if it stops being inlined or stops calling CallerSubsumes both this bit and the bit in CGBindingRoot can be removed.

> +++ b/dom/bindings/FakeString.h

This looks like it lost the original blame.  A better way to do this in terms of the VCS is to "hg cp BindingUtils.h FakeString.h" and then delete all the non-fakestring parts, so blame is preserved.

r=me with the above fixed.  I wish we didn't have this internal/external string nastiness.  :(

Comment 38

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 1: Remove redundant trailing spaces. v2

Comment 39

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 2: Support DOMString as Argument 2 of Document.createElement.

Comment 40

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 1: Remove redundant trailing spaces. (Final) r=bz

Comment 41

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 2: Modify CheckCustomElementName be 'const nsString*' and remove the const_cast. (Final) r=bz

Comment 42

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 3: Modify Codegen to generate including FakeString's binding files and fix build errors. v3

Comment 43

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 4: Support DOMString as Argument 2 of Document.createElement. v3

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Question, are the changes here small enough that we can take these to Aurora? That is where we need the fix too.

Comment 45

[John Dai[:jdai]]

Cancel the review first, since there are some build errors need to handle.
Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11f64d0fa72064158cc9965fd4e7d010bba654ee

Comment 46

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 3: Modify Codegen to generate including FakeString's binding files and fix build errors. v4

Comment 47

[John Dai[:jdai]]

I apologize for the review request spam. This version should be the final one. Those patches addressed comment #37 and comment #38.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d92d5d9344a98b5d1c1484a25db8bee2b844cfa0&filter-tier=1

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8787156 [details] [diff] [review]
Bug 1294100 - Part 1: Remove redundant trailing spaces. (Final) r=bz

r=me

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8787157 [details] [diff] [review]
Bug 1294100 - Part 2: Modify CheckCustomElementName be 'const nsString*' and remove the const_cast. (Final) r=bz

r=me

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8787520 [details] [diff] [review]
Bug 1294100 - Part 3: Modify Codegen to generate including FakeString's binding files and fix build errors. v4

>+        bindingDeclareHeaders["mozilla/dom/BindingUtils.h"] = any(d.isObject() for t in unionTypes \
>+                                                                  for d in t.flatMemberTypes)

You don't need the backslash there.  Please take it out.

r=me

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8787160 [details] [diff] [review]
Bug 1294100 - Part 4: Support DOMString as Argument 2 of Document.createElement. v3

>+  options.SetAsString().SetIsVoid(true);

No, that's not right.  That sets the string to null, which isn't even a valid value in this union.

I think just this:

  options.SetAsString();

is fine.  Or you could do:

  options.SetAsString().Truncate();

if you want to be more explicit about it being an empty string.  That applies to both callsites in this patch.

r=me with that fixed.

Comment 52

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 3: Modify Codegen to generate including FakeString's binding files and fix build errors. (Final) r=bz

Addressed comment #50.

Comment 53

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 4: Support DOMString as Argument 2 of Document.createElement. (Final) r=bz

Addressed comment #51.

Comment 54

[John Dai[:jdai]]

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=669528833a49f4cb1fddab660e3d9b47bbe3e563&filter-tier=1

Comment 55

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 3: Modify Codegen to generate including FakeString's binding files and fix build errors. (Final) r=bz

Comment 56

[Carsten Book [:Tomcat]]

has problems to apply 

renamed 1294100 -> bug_1294100_v5_2.patch
applying bug_1294100_v5_2.patch
patching file dom/base/nsDocument.cpp
Hunk #3 FAILED at 13445
1 out of 3 hunks FAILED -- saving rejects to file dom/base/nsDocument.cpp.rej
patching file dom/base/nsDocument.h
Hunk #2 succeeded at 1375 with fuzz 2 (offset -150 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory

Comment 57

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 1: Remove redundant trailing spaces. r=bz

Rebased my patch.

Comment 58

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 2: Modify CheckCustomElementName be 'const nsString*' and remove the const_cast. r=bz

Rebased my patch.

Comment 59

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 3: Modify Codegen to generate including FakeString's binding files and fix build errors. r=bz

Comment 60

[John Dai[:jdai]]

Attachment: Bug 1294100 - Part 4: Support DOMString as Argument 2 of Document.createElement. r=bz

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eebc85e916eb4161e31acd783bee8bcac0aa7bc4&filter-tier=1

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b43f4b166fb9
Part 1: Remove redundant trailing spaces. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae75b8c3fc50
Part 2: Modify CheckCustomElementName be 'const nsString*' and remove the const_cast. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e027eec0b61
Part 3: Modify Codegen to generate including FakeString's binding files and fix build errors. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe972bcc9ff
Part 4: Support DOMString as Argument 2 of Document.createElement. r=bz

Comment 62

[John Dai[:jdai]]

Attachment: (Aurora) Bug 1294100 - Part 1: Remove redundant trailing spaces. r=bz

Comment 63

[John Dai[:jdai]]

Attachment: (Aurora) Bug 1294100 - Part 2: Modify CheckCustomElementName be 'const nsString*' and remove the const_cast. r=bz

Comment 64

[John Dai[:jdai]]

Attachment: (Aurora) Bug 1294100 - Part 3: Modify Codegen to generate including FakeString's binding files and fix build errors. r=bz

Comment 65

[John Dai[:jdai]]

Attachment: (Aurora) Bug 1294100 - Part 4: Support DOMString as Argument 2 of Document.createElement. r=bz

Comment 66

[John Dai[:jdai]]

Aurora try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec6a7cbc0c4240fa914e6bb20b731a7e59bacce7&filter-tier=1

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b43f4b166fb9
https://hg.mozilla.org/mozilla-central/rev/ae75b8c3fc50
https://hg.mozilla.org/mozilla-central/rev/3e027eec0b61
https://hg.mozilla.org/mozilla-central/rev/cbe972bcc9ff

Comment 68

[John Dai[:jdai]]

Comment on attachment 8788760 [details] [diff] [review]
(Aurora) Bug 1294100 - Part 1: Remove redundant trailing spaces. r=bz

Approval Request Comment
[Feature/regressing bug #]: bug 1276579 introduced this bug.
[User impact if declined]:  Document.createElement() function may not work under some circumstances.
[Describe test coverage new/current, TreeHerder]:The patch has been tested
manually and it also has an automated test in dom/tests/mochitest/webcomponents/test_document_register.html.
[Risks and why]: Extremely low. It affects a single function call.
[String/UUID change made/needed]: none.

Comment 69

[John Dai[:jdai]]

Comment on attachment 8788761 [details] [diff] [review]
(Aurora) Bug 1294100 - Part 2: Modify CheckCustomElementName be 'const nsString*' and remove the const_cast. r=bz

Approval Request Comment
[Feature/regressing bug #]: bug 1276579 introduced this bug.
[User impact if declined]:  Document.createElement() function may not work under some circumstances.
[Describe test coverage new/current, TreeHerder]:The patch has been tested
manually and it also has an automated test in dom/tests/mochitest/webcomponents/test_document_register.html.
[Risks and why]: Extremely low. It affects a single function call.
[String/UUID change made/needed]: none.

Comment 70

[John Dai[:jdai]]

Comment on attachment 8788762 [details] [diff] [review]
(Aurora) Bug 1294100 - Part 3: Modify Codegen to generate including FakeString's binding files and fix build errors. r=bz

Approval Request Comment
[Feature/regressing bug #]: bug 1276579 introduced this bug.
[User impact if declined]:  Document.createElement() function may not work under some circumstances.
[Describe test coverage new/current, TreeHerder]:The patch has been tested
manually and it also has an automated test in dom/tests/mochitest/webcomponents/test_document_register.html.
[Risks and why]: Extremely low. It affects a single function call.
[String/UUID change made/needed]: none.

Comment 71

[John Dai[:jdai]]

Comment on attachment 8788763 [details] [diff] [review]
(Aurora) Bug 1294100 - Part 4: Support DOMString as Argument 2 of Document.createElement. r=bz

Approval Request Comment
[Feature/regressing bug #]: bug 1276579 introduced this bug.
[User impact if declined]:  Document.createElement() function may not work under some circumstances.
[Describe test coverage new/current, TreeHerder]:The patch has been tested
manually and it also has an automated test in dom/tests/mochitest/webcomponents/test_document_register.html.
[Risks and why]: Extremely low. It affects a single function call.
[String/UUID change made/needed]: none.

Comment 72

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Pouyan, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 73

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8788761 [details] [diff] [review]
(Aurora) Bug 1294100 - Part 2: Modify CheckCustomElementName be 'const nsString*' and remove the const_cast. r=bz

Seems like a recent regression, Aurora50+

Comment 74

[Pouyan]

:ritu, I can confirm this as fixed on Firefox Nighty 51.0a1 (2016-09-08) (64-bit) OSx.

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eb920f64d303
https://hg.mozilla.org/releases/mozilla-aurora/rev/1e5864aa60cf
https://hg.mozilla.org/releases/mozilla-aurora/rev/5fcc8344819b
https://hg.mozilla.org/releases/mozilla-aurora/rev/7a777f24e7f0

Comment 76

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Pouyan from comment #74)
> :ritu, I can confirm this as fixed on Firefox Nighty 51.0a1 (2016-09-08)
> (64-bit) OSx.

Great! Thanks a lot.