Bug 187682 - Add an SPI hook to allow clients to yield document parsing and script execution
Summary: Add an SPI hook to allow clients to yield document parsing and script execution
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-14 17:07 PDT by Wenson Hsieh
Modified: 2018-07-19 11:44 PDT (History)
16 users (show)

See Also:


Attachments
Patch (40.05 KB, patch)
2018-07-14 17:49 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
First pass (40.06 KB, patch)
2018-07-14 18:14 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Address feedback. (42.79 KB, patch)
2018-07-16 12:59 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Address feedback. (42.30 KB, patch)
2018-07-16 13:02 PDT, Wenson Hsieh
rniwa: review+
Details | Formatted Diff | Diff
Patch for EWS (59.69 KB, patch)
2018-07-17 08:55 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch for landing (59.70 KB, patch)
2018-07-17 10:43 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2018-07-14 17:07:49 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-07-14 17:08:19 PDT
<rdar://problem/42207453>
Comment 2 Wenson Hsieh 2018-07-14 17:49:54 PDT
Created attachment 345047 [details]
Patch
Comment 3 Wenson Hsieh 2018-07-14 18:14:03 PDT
Created attachment 345048 [details]
First pass
Comment 4 Simon Fraser (smfr) 2018-07-16 08:32:04 PDT
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.
Comment 5 Wenson Hsieh 2018-07-16 09:10:27 PDT
(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?
Comment 6 Ryosuke Niwa 2018-07-16 11:09:33 PDT
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.
Comment 7 Wenson Hsieh 2018-07-16 11:23:08 PDT
(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.
Comment 8 Wenson Hsieh 2018-07-16 12:59:25 PDT
Created attachment 345113 [details]
Address feedback.
Comment 9 Wenson Hsieh 2018-07-16 13:02:09 PDT
Created attachment 345114 [details]
Address feedback.
Comment 10 Ryosuke Niwa 2018-07-16 21:15:41 PDT
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.
Comment 11 Wenson Hsieh 2018-07-16 22:12:54 PDT
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 ^)
Comment 12 Wenson Hsieh 2018-07-17 08:55:53 PDT
Created attachment 345155 [details]
Patch for EWS
Comment 13 Wenson Hsieh 2018-07-17 10:43:15 PDT
Created attachment 345167 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2018-07-17 11:23:43 PDT
Comment on attachment 345167 [details]
Patch for landing

Clearing flags on attachment: 345167

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