Bug 227846 – document.readyState should be "complete" after calling DOMParser.parseFromString()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

document.readyState should be "complete" after calling DOMParser.parseFromString().

This is causing the following WPT test to fail in WebKit:
http://wpt.live/domparsing/xmldomparser.html

Both Gecko and Blink report the correct readyState here.

[Chris Dumez]

Created attachment 433246 [details]
Patch

[Chris Dumez]

Created attachment 433248 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 433248 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:3106
> +        setReadyState(Complete);

Does this also impact document.close?

[Sam Weinig]

Comment on attachment 433248 [details]
Patch

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

> Source/WebCore/ChangeLog:26
> +        * dom/Document.cpp:
> +        (WebCore::Document::explicitClose):
> +        explicitClose() normally calls checkCompleted() which calls FrameLoader::checkCompleted(),
> +        which ends up setting the document's ready state to "complete" and then calling
> +        Document::implicitClose(). However, when the document has no frame (which is the case
> +        for a document just created via DOMParser.parseFromString()), we would call
> +        Document::implicitClose() directly, since we don't have a FrameLoader. As a result,
> +        the document's ready state would stay "interactive". To address the issue, we now set
> +        the document's ready state to "complete" before calling implicitClose(), similarly to
> +        what FrameLoader::checkCompleted() would have done.

Seems good. I wish we had a better model of dealing with frameless documents, so we aren't duplicating functionality.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 433248 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433248&action=review
> 
> > Source/WebCore/dom/Document.cpp:3106
> > +        setReadyState(Complete);
> 
> Does this also impact document.close?

Not super familiar with `document.close()` but looking at the code, it seems to only have an impact on documents created by JS because of this early return:
```
    if (!scriptableDocumentParser() || !scriptableDocumentParser()->wasCreatedByScript() || !scriptableDocumentParser()->isParsing())
        return;
```

Documents created by scripts should now have a readyState that is "complete" after my patch so calling close() on the document later on should not make a difference on the readyState since it is already "complete". Am I missing something?

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #5)
> (In reply to Ryosuke Niwa from comment #3)
> > Comment on attachment 433248 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=433248&action=review
> > 
> > > Source/WebCore/dom/Document.cpp:3106
> > > +        setReadyState(Complete);
> > 
> > Does this also impact document.close?
> 
> Not super familiar with `document.close()` but looking at the code, it seems
> to only have an impact on documents created by JS because of this early
> return:
> ```
>     if (!scriptableDocumentParser() ||
> !scriptableDocumentParser()->wasCreatedByScript() ||
> !scriptableDocumentParser()->isParsing())
>         return;
> ```
> 
> Documents created by scripts should now have a readyState that is "complete"
> after my patch so calling close() on the document later on should not make a
> difference on the readyState since it is already "complete". Am I missing
> something?

How about document.implementation.createDocument/createHTMLDocument?

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #6)
> (In reply to Chris Dumez from comment #5)
> > (In reply to Ryosuke Niwa from comment #3)
> > > Comment on attachment 433248 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=433248&action=review
> > > 
> > > > Source/WebCore/dom/Document.cpp:3106
> > > > +        setReadyState(Complete);
> > > 
> > > Does this also impact document.close?
> > 
> > Not super familiar with `document.close()` but looking at the code, it seems
> > to only have an impact on documents created by JS because of this early
> > return:
> > ```
> >     if (!scriptableDocumentParser() ||
> > !scriptableDocumentParser()->wasCreatedByScript() ||
> > !scriptableDocumentParser()->isParsing())
> >         return;
> > ```
> > 
> > Documents created by scripts should now have a readyState that is "complete"
> > after my patch so calling close() on the document later on should not make a
> > difference on the readyState since it is already "complete". Am I missing
> > something?
> 
> How about document.implementation.createDocument/createHTMLDocument?

I think I'll add some test and make sure our behavior is consistent with other engines.

[Chris Dumez]

Created attachment 433254 [details]
Patch

[EWS]

Committed r279803 (239567@main): <https://commits.webkit.org/239567@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433254 [details].

[Radar WebKit Bug Importer]

<rdar://problem/80408826>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 227850

[Chris Dumez]

Created attachment 433272 [details]
Patch

[EWS]

Committed r279814 (239575@main): <https://commits.webkit.org/239575@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433272 [details].