[Webkit-unassigned] [Bug 42365] HTML5 Parser: document.write in a asynchronous script which is specified to load before page finish blows away document

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42365





--- Comment #12 from Adam Barth <abarth at webkit.org>  2010-07-22 18:08:30 PST ---
> I'm having trouble understanding this patch.

Eric and I discussed the patch verbally.

> LayoutTests/http/tests/misc/write-from-dom-script.html:9
>  +  script.src = "data:text/javascript,document.write('FAIL');document.close();";
> It's sad that this script would "PASS" if your script had an error.  Would be better if the script set some global that was also checked and could cause FAIL.

Fixed.

> LayoutTests/http/tests/misc/write-while-waiting.html:8
>  +  setTimeout("document.write('PASS');document.close();", 100);
> Would be nice if you could explain in the ChangeLog why this is right?  Maybe you did and I didn't understand.

I added more details to the ChangeLog.  Ian's trying to narrowly address the compatibility problem.

> WebCore/dom/Document.cpp:366
>  +      , m_isWriteNeutralised(false)
> It's sad that this is on Document and not DocumentWriter or something else.  Document is such a dumping ground.

Of all the state on document, this one is very appropriate.  The spec stores the state on Document and it's read by Document::write, which seems to really belong on document.

> WebCore/dom/Document.cpp:2020
>  +      bool insertionPointIsUndefined = !m_parser || !m_parser->hasInsertionPoint();
> This would have been more clear (to me) as:
> bool hasInsertionPoint = m_parser && m_parser->hasInsertionPoint();

Done.

> WebCore/dom/Document.cpp:2023
>  +      if (insertionPointIsUndefined && isWriteNeutralised())
> Then this becomes !hasInsertionPoint

Yep.

> WebCore/dom/Document.h:1005
>  +      bool isWriteNeutralised() const { return m_isWriteNeutralised; }
> I wish the spec called this "writeDisabled", Neutralized is a poor name IMO.
> Hard to spell, hard to type, hard for our numerous non-english speakers to understand.

Done.  I chose the name to match the spec.  It still has a hyperlink to the spec, so folks should be able to figure it out.

> WebCore/dom/DocumentParser.h:44
>  +      virtual bool hasInsertionPoint() { return true; }
> So what does this mean when true?  Who should override it?  If it's true, you'll always get an insert call?

We discussed this over the phone.  The default should be false, but that messes up some SVG font code.  I'll make another patch that changes the default to false and fixes the SVG font code to use the right APIs.

> WebCore/dom/ScriptElement.cpp:59
>  +      // If the element's Document has an active parser, and the parser's script
> This comment makes it seem like this should not go inside ScriptElement.

I didn't understand this comment.  It's in the "script element" section of the spec and it's manipulating some part of the script element's state...  Putting the code here seems to make sense.

> WebCore/dom/ScriptElement.cpp:61
>  +      // "parser-inserted" flag set, the user agent must set the element's
> Or maybe that the Document should be notified that the script was inserted and it should make a decision to nueter itself or not.

I think this comment stems from your confusing about how the dataflow works.  The document does get notified, but when the script runs.  Here we're remembering what life was like when we were inserted into the document.

> WebCore/dom/ScriptElement.cpp: 
>  +  static inline bool useHTML5Parser(Document* document)
> This was previous dead code?

Yes.  Technically this change is unrelated, but I just saw it and removed it.

> WebCore/dom/ScriptElement.cpp:203
>  +          // flag was set as being itself "write-neutralised". Let neutralised doc
> God this is an awful name.  I need to complain to Hixie.
> Done: http://www.w3.org/Bugs/Public/show_bug.cgi?id=10228
> 
> Btw, my spelling corrector seems to think it's "neutralized" not "neutralised".  Yet another reason why that name needs to go.

The name is gone, from the code at least.

> WebCore/html/HTMLDocumentParser.cpp:218
>  +      if (m_scriptRunner && m_scriptRunner->inScriptExecution())
> This needs a comment to explain why.  It's not clear why "inScirptExecution" means there is an insertion point.  Why would that even be needed?  Can't we check m_input?

I've changed this to get the information from m_input.  I've also renamed HTMLScriptRunner::inScriptExecution to something more informative.

> WebCore/html/HTMLDocumentParser.cpp:220
>  +      // FIXME: Checking whether we've seen end of file isn't quite right when
> I don't undersand this FIXME.  Can you explain better?

I've removed it.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.