Bug 189373 - No-op document.open() calls should not have any side effects
Summary: No-op document.open() calls should not have any side effects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 189863
Blocks:
  Show dependency treegraph
 
Reported: 2018-09-06 14:26 PDT by Timothy Gu
Modified: 2018-09-24 15:56 PDT (History)
11 users (show)

See Also:


Attachments
WIP Patch (2.53 KB, patch)
2018-09-21 16:07 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (4.56 KB, patch)
2018-09-24 12:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Gu 2018-09-06 14:26:32 PDT
See https://github.com/WebKit/webkit/blob/ba62d1cc832b5c357da6532708c0db83a2d8216e/Source/WebCore/dom/Document.cpp#L2670-L2686:

    if (responsibleDocument) {
        setURL(responsibleDocument->url());
        setCookieURL(responsibleDocument->cookieURL());
        setSecurityOriginPolicy(responsibleDocument->securityOriginPolicy());
    }

    if (m_frame) {
        if (ScriptableDocumentParser* parser = scriptableDocumentParser()) {
            if (parser->isParsing()) {
                // FIXME: HTML5 doesn't tell us to check this, it might not be correct.
                if (parser->isExecutingScript())
                    return;

                if (!parser->wasCreatedByScript() && parser->hasInsertionPoint())
                    return;
            }
        }
        ...
    }

The URL updates should not happen until the active parser checks. Per https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#document-open-steps, URL updating happens in step 11, while the parser returns are step 5.

>  5. If document has an active parser whose script nesting level is greater than 0, then return document.
>
> ...
>
> 11. If document is fully active, then:
>    1. Let newURL be a copy of entryDocument's URL.
>    2. If entryDocument is not document, then set newURL's fragment to null.
>    3. Run the URL and history update steps with document and newURL.

Test: https://github.com/web-platform-tests/wpt/blob/master/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/bailout-side-effects-synchronous-script.window.js
Comment 1 Radar WebKit Bug Importer 2018-09-09 13:28:52 PDT
<rdar://problem/44282702>
Comment 2 Chris Dumez 2018-09-21 16:07:48 PDT
Created attachment 350450 [details]
WIP Patch
Comment 3 Chris Dumez 2018-09-24 12:26:55 PDT
Created attachment 350665 [details]
Patch
Comment 4 Geoffrey Garen 2018-09-24 15:15:26 PDT
Comment on attachment 350665 [details]
Patch

r=me
Comment 5 Chris Dumez 2018-09-24 15:19:08 PDT
Thanks for the bug report and the investigation Timothy. This was extremely helpful.
Comment 6 WebKit Commit Bot 2018-09-24 15:56:04 PDT
Comment on attachment 350665 [details]
Patch

Clearing flags on attachment: 350665

Committed r236433: <https://trac.webkit.org/changeset/236433>
Comment 7 WebKit Commit Bot 2018-09-24 15:56:06 PDT
All reviewed patches have been landed.  Closing bug.