Closed Bug 1294100 Opened 3 years ago Closed 3 years ago

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

Categories

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

50 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox50 + fixed
firefox51 + verified

People

(Reporter: me, Assigned: jdai)

References

Details

(Keywords: regression, Whiteboard: dom-ce-m1)

Attachments

(8 files, 21 obsolete files)

880 bytes, patch
jdai
: review+
Details | Diff | Splinter Review
7.85 KB, patch
jdai
: review+
Details | Diff | Splinter Review
118.36 KB, patch
jdai
: review+
Details | Diff | Splinter Review
6.44 KB, patch
jdai
: review+
Details | Diff | Splinter Review
914 bytes, patch
Details | Diff | Splinter Review
7.81 KB, patch
Details | Diff | Splinter Review
118.51 KB, patch
Details | Diff | Splinter Review
6.49 KB, patch
Details | Diff | Splinter Review
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
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
OS: Linux → Mac OS X
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?
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Flags: needinfo?(bugs)
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86_64 → All
See Also: → 856140, 1276579
Tooru, how many cases have we hit thus far?

Pouyan, is this something you can fix?
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...
I filed https://github.com/Polymer/docs/issues/1705. Mike, you might want to poke them too if you have a channel.
Flags: needinfo?(miket)
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.
Pouyan, thank you, appreciated. Though it being in a product suggests we likely have to make this work... Sigh.
I also filed https://github.com/w3c/webcomponents/issues/544 so that we can consider a change to the standard.
[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
Flags: needinfo?(bugs)
Hi John, is this something you could help?
Flags: needinfo?(jdai)
Assignee: nobody → jdai
Flags: needinfo?(jdai)
Hi John, any update here?
Flags: needinfo?(jdai)
Attached patch wip (obsolete) — Splinter Review
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'
Flags: needinfo?(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'
>
Attached patch Part 1: wip (obsolete) — Splinter Review
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.
Attachment #8781822 - Attachment is obsolete: true
Attachment #8781956 - Flags: feedback?(bugs)
Attached patch Part 2: wip (obsolete) — Splinter Review
This patch mainly fix comment #11 errors.
Attachment #8781957 - Flags: feedback?(bugs)
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.
Attachment #8781957 - Flags: feedback?(bugs) → feedback+
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?
Attachment #8781956 - Flags: feedback?(bugs) → feedback+
(sorry, was on PTO. clearing ni? because it seems the scope here is beyond tech evangelism)
Flags: needinfo?(miket)
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
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?
Flags: needinfo?(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.
Flags: needinfo?(jdai)
Attachment #8781956 - Attachment is obsolete: true
Attachment #8781957 - Attachment is obsolete: true
Those patches aren't ready to review, it only can pass build on linux.
(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.
(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.
> 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.
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
Attachment #8783841 - Attachment is obsolete: true
Attachment #8783842 - Attachment is obsolete: true
Attachment #8783843 - Attachment is obsolete: true
Attachment #8783844 - Attachment is obsolete: true
Attachment #8783845 - Attachment is obsolete: true
Attachment #8784325 - Flags: review?(bugs)
> 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 on attachment 8784325 [details] [diff] [review]
Bug 1294100 - Support DOMString as Argument 2 of Document.createElement.

Let me do it in a right way.
Attachment #8784325 - Flags: review?(bugs)
Whiteboard: dom-ce-m1
Attachment #8784325 - Attachment is obsolete: true
Attachment #8785903 - Flags: review?(bzbarsky)
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
Attachment #8785904 - Flags: review?(bzbarsky)
Tracking 51+ for web compat.
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.
Attachment #8785903 - Flags: review?(bzbarsky) → review-
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.  :(
Attachment #8785904 - Flags: review?(bzbarsky) → review+
Attachment #8785903 - Attachment is obsolete: true
Attachment #8785904 - Attachment is obsolete: true
Attachment #8787101 - Attachment is obsolete: true
Attachment #8787102 - Attachment is obsolete: true
Attachment #8787156 - Flags: review?(bzbarsky)
Attachment #8787156 - Flags: review?(bzbarsky)
Attachment #8787157 - Flags: review?(bzbarsky)
Attachment #8787159 - Flags: review?(bzbarsky)
Attachment #8787160 - Flags: review?(bzbarsky)
Question, are the changes here small enough that we can take these to Aurora? That is where we need the fix too.
Cancel the review first, since there are some build errors need to handle.
Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11f64d0fa72064158cc9965fd4e7d010bba654ee
Attachment #8787156 - Flags: review?(bzbarsky)
Attachment #8787157 - Flags: review?(bzbarsky)
Attachment #8787160 - Flags: review?(bzbarsky)
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 on attachment 8787156 [details] [diff] [review]
Bug 1294100 - Part 1: Remove redundant trailing spaces. (Final) r=bz

r=me
Attachment #8787156 - Flags: review?(bzbarsky) → review+
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
Attachment #8787157 - Flags: review?(bzbarsky) → review+
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
Attachment #8787520 - Flags: review?(bzbarsky) → review+
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.
Attachment #8787160 - Flags: review?(bzbarsky) → review+
Attachment #8787156 - Attachment description: Bug 1294100 - Part 1: Remove redundant trailing spaces. v3 → Bug 1294100 - Part 1: Remove redundant trailing spaces. (Final) r=bz
Attachment #8787157 - Attachment description: Bug 1294100 - Part 2: Modify CheckCustomElementName be 'const nsString*' and remove the const_cast. v3 → Bug 1294100 - Part 2: Modify CheckCustomElementName be 'const nsString*' and remove the const_cast. (Final) r=bz
Keywords: checkin-needed
Keywords: checkin-needed
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
Flags: needinfo?(jdai)
Keywords: checkin-needed
Rebased my patch.
Attachment #8787156 - Attachment is obsolete: true
Attachment #8787157 - Attachment is obsolete: true
Attachment #8788031 - Attachment is obsolete: true
Attachment #8788082 - Attachment is obsolete: true
Flags: needinfo?(jdai)
Attachment #8788354 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Attachment #8788762 - Attachment description: Aurora: Bug 1294100 - Part 3: Modify Codegen to generate including FakeString's binding files and fix build errors. r=bz → (Aurora) Bug 1294100 - Part 3: Modify Codegen to generate including FakeString's binding files and fix build errors. r=bz
Attachment #8788761 - Attachment description: Bug 1294100 - Part 2: Modify CheckCustomElementName be 'const nsString*' and remove the const_cast. (aurora) r=bz → (Aurora) Bug 1294100 - Part 2: Modify CheckCustomElementName be 'const nsString*' and remove the const_cast. r=bz
Attachment #8788760 - Attachment description: Bug 1294100 - Part 1: Remove redundant trailing spaces. (aurora) r=bz → (Aurora) Bug 1294100 - Part 1: Remove redundant trailing spaces. r=bz
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.
Attachment #8788760 - Flags: approval-mozilla-aurora?
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.
Attachment #8788761 - Flags: approval-mozilla-aurora?
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.
Attachment #8788762 - Flags: approval-mozilla-aurora?
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.
Attachment #8788763 - Flags: approval-mozilla-aurora?
Hello Pouyan, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(popeno2003)
Attachment #8788760 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: regression
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+
Attachment #8788761 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8788762 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8788763 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
:ritu, I can confirm this as fixed on Firefox Nighty 51.0a1 (2016-09-08) (64-bit) OSx.
Flags: needinfo?(popeno2003)
(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.
Status: RESOLVED → VERIFIED
Blocks: 1276579
See Also: 1276579
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.