Bug 187682 – Add an SPI hook to allow clients to yield document parsing and script execution

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Wenson Hsieh]

The intended use case is to allow Safari modal sheet on watchOS to avoid carrying out performing costly work on the main document after we've already determined that the page should be represented using Reader.

[Radar WebKit Bug Importer]

<rdar://problem/42207453>

[Wenson Hsieh]

Created attachment 345047 [details]
Patch

[Wenson Hsieh]

Created attachment 345048 [details]
First pass

[Simon Fraser (smfr)]

Comment on attachment 345048 [details]
First pass

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

> Source/WebCore/dom/Document.h:1276
> +    class ParserYieldToken {
> +    public:
> +        WEBCORE_EXPORT ParserYieldToken(Document&);
> +        WEBCORE_EXPORT ~ParserYieldToken();
> +
> +    private:
> +        Ref<Document> m_document;
> +    };

Does this thing really need to keep a Ref to the Document? It seems like this would possibly keep the Document alive longer than normal (especially since this is wrapped in an ObjC class that clients can make mistakes with). This goes against all the Document lifetime fixes that I've been making.

[Wenson Hsieh]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 345048 [details]
> First pass
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345048&action=review
> 
> > Source/WebCore/dom/Document.h:1276
> > +    class ParserYieldToken {
> > +    public:
> > +        WEBCORE_EXPORT ParserYieldToken(Document&);
> > +        WEBCORE_EXPORT ~ParserYieldToken();
> > +
> > +    private:
> > +        Ref<Document> m_document;
> > +    };
> 
> Does this thing really need to keep a Ref to the Document? It seems like
> this would possibly keep the Document alive longer than normal (especially
> since this is wrapped in an ObjC class that clients can make mistakes with).
> This goes against all the Document lifetime fixes that I've been making.

Fair point. What about a weak pointer instead?

[Ryosuke Niwa]

Comment on attachment 345048 [details]
First pass

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

r- due to perf concerns.

> Source/WebCore/html/parser/HTMLParserScheduler.cpp:118
> +    if (UNLIKELY(m_parser.document()->hasActiveParserYieldToken()))

We can't load parser & document like this. This will regress perf.
Add a flag on the HTMLParserScheduler itself and have Document's method update that instead.

> Source/WebCore/html/parser/HTMLParserScheduler.h:-66
> -    bool shouldYieldBeforeToken(PumpSession& session)

We can't move this out of the header. It's very important that this function is inlined.
Otherwise, we'd regress the perf.

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #6)
> Comment on attachment 345048 [details]
> First pass
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345048&action=review
> 
> r- due to perf concerns.
> 
> > Source/WebCore/html/parser/HTMLParserScheduler.cpp:118
> > +    if (UNLIKELY(m_parser.document()->hasActiveParserYieldToken()))
> 
> We can't load parser & document like this. This will regress perf.
> Add a flag on the HTMLParserScheduler itself and have Document's method
> update that instead.

Good call!

> 
> > Source/WebCore/html/parser/HTMLParserScheduler.h:-66
> > -    bool shouldYieldBeforeToken(PumpSession& session)
> 
> We can't move this out of the header. It's very important that this function
> is inlined.
> Otherwise, we'd regress the perf.

Understood. I'll consult a flag on HTMLParserScheduler instead.

[Wenson Hsieh]

Created attachment 345113 [details]
Address feedback.

[Wenson Hsieh]

Created attachment 345114 [details]
Address feedback.

[Ryosuke Niwa]

Comment on attachment 345114 [details]
Address feedback.

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

> Source/WebCore/dom/Document.h:1269
> +    class ParserYieldToken {

I don't think this needs to be an inner class of Document.

> Source/WebCore/html/parser/HTMLParserScheduler.h:90
> +    void didBeginYieldingParser() { m_documentHasActiveParserYieldTokens = true; }
> +    void didEndYieldingParser() { m_documentHasActiveParserYieldTokens = false; }

Let's assert that m_documentHasActiveParserYieldTokens is false & true respectively here.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ParserYieldTokenTests.mm:93
> +    waitForDelay(1_s);

Instead of waiting for 1s like this, can we have:
1. <script src="a.js" async></a>
2. <script>fetch('a.js').then(/* 3. post-to-test */)</script>
and make sure (2) happens before (1) runs?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ParserYieldTokenTests.mm:110
> +    waitForDelay(0.5_s);

Ditto.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ParserYieldTokenTests.mm:134
> +    waitForDelay(0.5_s);

Ditto.

[Wenson Hsieh]

Comment on attachment 345114 [details]
Address feedback.

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

>> Source/WebCore/dom/Document.h:1269
>> +    class ParserYieldToken {
> 
> I don't think this needs to be an inner class of Document.

Fair enough — I moved this outside of the Document class.

>> Source/WebCore/html/parser/HTMLParserScheduler.h:90
>> +    void didEndYieldingParser() { m_documentHasActiveParserYieldTokens = false; }
> 
> Let's assert that m_documentHasActiveParserYieldTokens is false & true respectively here.

Ok! Added the assertions.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ParserYieldTokenTests.mm:93
>> +    waitForDelay(1_s);
> 
> Instead of waiting for 1s like this, can we have:
> 1. <script src="a.js" async></a>
> 2. <script>fetch('a.js').then(/* 3. post-to-test */)</script>
> and make sure (2) happens before (1) runs?

Ryosuke and I discussed this in person — this doesn't really fulfill the same purpose as waiting 1 sec. here, which is to ensure that the document load (DOMContentLoaded) as well as page load are deferred while the token is held (hence, why we check that -finishedDocumentLoad and -finishedLoad are still false afterwards). We also couldn't think of a great way to test this behavior without using a timeout like this.

That being said, this is still a good scenario to test, so I'll add it as a fourth test case in this suite.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ParserYieldTokenTests.mm:110
>> +    waitForDelay(0.5_s);
> 
> Ditto.

(See above ^)

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ParserYieldTokenTests.mm:134
>> +    waitForDelay(0.5_s);
> 
> Ditto.

(See above ^)

[Wenson Hsieh]

Created attachment 345155 [details]
Patch for EWS

[Wenson Hsieh]

Created attachment 345167 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 345167 [details]
Patch for landing

Clearing flags on attachment: 345167

Committed r233891: <https://trac.webkit.org/changeset/233891>