Closed Bug 569538 Opened 10 years ago Closed 10 years ago

Parser should tell document when it is creating elements because of document.write or innerHTML

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: hsivonen)

References

Details

Attachments

(1 file, 2 obsolete files)

.
There should be probably separate flag for document.write and innerHTML
Blocks: 566879
Attached patch Suggested implementation (obsolete) — Splinter Review
Like this?

This puts the flags on the document. It might be more logical to put them on the parser and have the document ask its parser for the flags. That would require virtuals, though. This doesn't.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #448736 - Flags: feedback?(Olli.Pettay)
nsIDocument could have getters for the flags.

Does the implementation work if for example document.write() causes recursive
document.write calls?
I assume external <script> inside document.write is handled by the ParseUntilBlocked stuff which gets called again once that script is processed?
Attached patch Suggested implementation, v2 (obsolete) — Splinter Review
(In reply to comment #3)
> nsIDocument could have getters for the flags.

Added.

> Does the implementation work if for example document.write() causes recursive
> document.write calls?

I thought it did, but it didn't. Fixed.

As an alternative to putting these flags on the document, the PRBool aFromParser flag on creator functions could be changed into a PRUint32 aFlags with distinct bits from network, document.write and fragment parser. Would that be worthwhile? Would that address the use cases?
Attachment #448736 - Attachment is obsolete: true
Attachment #448736 - Flags: feedback?(Olli.Pettay)
Changing the aFromParser to aFlags would be good, I think.
No need to change nsIDocument in that case.
I turned aFromParser into an PRUint32 but I didn't rename it, as it still indicates whether the call is from parser. 0 means what it meant before. The lowest 3 bits now distinguish between network, document.write and fragment.

The XML fragment sink claims that its element don't come from parser before and after this patch. I think I don't like that, but I think changing it is out of the scope of this bug.

I didn't change all the callers, since they are numerous and passing PR_FALSE and PR_TRUE do the right thing, except in the old HTML parser, but making it distinguish document.write would be annoying to say the least.
Attachment #448980 - Attachment is obsolete: true
Oh, and I didn't use an enum, because an enum would make existing uses of aFromParser (!aFromParser) non-idiomatic.
Attachment #449221 - Flags: review?(Olli.Pettay)
Comment on attachment 449221 [details] [diff] [review]
Different suggested implementation


> class nsHTMLAudioElement : public nsHTMLMediaElement,
>                            public nsIDOMHTMLAudioElement,
>                            public nsIJSNativeInitializer
> {
> public:
>-  nsHTMLAudioElement(nsINodeInfo *aNodeInfo, PRBool aFromParser = PR_FALSE);
>+  nsHTMLAudioElement(nsINodeInfo *aNodeInfo, PRUint32 aFromParser = PR_FALSE);
s/PR_FALSE/0/

> class nsHTMLMediaElement : public nsGenericHTMLElement,
>                            public nsIObserver
> {
>   typedef mozilla::layers::ImageContainer ImageContainer;
> 
> public:
>-  nsHTMLMediaElement(nsINodeInfo *aNodeInfo, PRBool aFromParser = PR_FALSE);
>+  nsHTMLMediaElement(nsINodeInfo *aNodeInfo, PRUint32 aFromParser = PR_FALSE);
s/PR_FALSE/0/

> class nsHTMLVideoElement : public nsHTMLMediaElement,
>                            public nsIDOMHTMLVideoElement
> {
> public:
>-  nsHTMLVideoElement(nsINodeInfo *aNodeInfo, PRBool aFromParser = PR_FALSE);
>+  nsHTMLVideoElement(nsINodeInfo *aNodeInfo, PRUint32 aFromParser = PR_FALSE);
s/PR_FALSE/0/

> #define NS_DECLARE_NS_NEW_HTML_ELEMENT(_elementName)              \
> nsGenericHTMLElement*                                             \
> NS_NewHTML##_elementName##Element(nsINodeInfo *aNodeInfo,         \
>-                                  PRBool aFromParser = PR_FALSE);
>+                                  PRUint32 aFromParser = PR_FALSE);
>
s/PR_FALSE/0/

 
> #define NS_DECLARE_NS_NEW_HTML_ELEMENT_AS_SHARED(_elementName)    \
> inline nsGenericHTMLElement*                                      \
> NS_NewHTML##_elementName##Element(nsINodeInfo *aNodeInfo,         \
>-                                  PRBool aFromParser = PR_FALSE)  \
>+                                  PRUint32 aFromParser = PR_FALSE)\
s/PR_FALSE/0/
> class nsHTMLObjectElement : public nsGenericHTMLFormElement,
>                             public nsObjectLoadingContent,
>                             public nsIDOMHTMLObjectElement
> #ifdef MOZ_SVG
>                             , public nsIDOMGetSVGDocument
> #endif
> {
> public:
>-  nsHTMLObjectElement(nsINodeInfo *aNodeInfo, PRBool aFromParser = PR_FALSE);
>+  nsHTMLObjectElement(nsINodeInfo *aNodeInfo, PRUint32 aFromParser = PR_FALSE);
s/PR_FALSE/0/

> class nsHTMLSelectElement : public nsGenericHTMLFormElement,
>                             public nsIDOMHTMLSelectElement,
>                             public nsIDOMNSHTMLSelectElement,
>                             public nsISelectElement
> {
> public:
>-  nsHTMLSelectElement(nsINodeInfo *aNodeInfo, PRBool aFromParser = PR_FALSE);
>+  nsHTMLSelectElement(nsINodeInfo *aNodeInfo, PRUint32 aFromParser = PR_FALSE);
s/PR_FALSE/0/
>@@ -62,17 +62,17 @@ class nsHTMLSharedObjectElement : public
>                                   public nsIDOMHTMLAppletElement,
>                                   public nsIDOMHTMLEmbedElement
> #ifdef MOZ_SVG
>                                   , public nsIDOMGetSVGDocument
> #endif
> {
> public:
>   nsHTMLSharedObjectElement(nsINodeInfo *aNodeInfo,
>-                            PRBool aFromParser = PR_FALSE);
>+                            PRUint32 aFromParser = PR_FALSE);
s/PR_FALSE/0/
> class nsHTMLTextAreaElement : public nsGenericHTMLFormElement,
>                               public nsIDOMHTMLTextAreaElement,
>                               public nsIDOMNSHTMLTextAreaElement,
>                               public nsITextControlElement,
>                               public nsIDOMNSEditableElement,
>                               public nsStubMutationObserver
> {
> public:
>-  nsHTMLTextAreaElement(nsINodeInfo *aNodeInfo, PRBool aFromParser = PR_FALSE);
>+  nsHTMLTextAreaElement(nsINodeInfo *aNodeInfo, PRUint32 aFromParser = PR_FALSE);
s/PR_FALSE/0/


>diff --git a/parser/html/nsHtml5TreeBuilderCppSupplement.h b/parser/html/nsHtml5TreeBuilderCppSupplement.h
>--- a/parser/html/nsHtml5TreeBuilderCppSupplement.h
>+++ b/parser/html/nsHtml5TreeBuilderCppSupplement.h
>@@ -86,17 +86,21 @@ nsHtml5TreeBuilder::createElement(PRInt3
>   NS_PRECONDITION(aNamespace == kNameSpaceID_XHTML || 
>                   aNamespace == kNameSpaceID_SVG || 
>                   aNamespace == kNameSpaceID_MathML,
>                   "Bogus namespace.");
> 
>   nsIContent** content = AllocateContentHandle();
>   nsHtml5TreeOperation* treeOp = mOpQueue.AppendElement();
>   NS_ASSERTION(treeOp, "Tree op allocation failed.");
>-  treeOp->Init(aNamespace, aName, aAttributes, content);
>+  treeOp->Init(aNamespace,
>+               aName,
>+               aAttributes,
>+               content,
>+               !!mSpeculativeLoadStage);
Could you add some comment why !!mSpeculativeLoadStage.
Attachment #449221 - Flags: review?(Olli.Pettay) → review+
Thanks. Landed with review comments addressed:
http://hg.mozilla.org/mozilla-central/rev/957d9d154f8c
http://hg.mozilla.org/mozilla-central/rev/6dff4175b408
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.