Bug 187682

<rdar://problem/42207453>

Created attachment 345047 [details] Patch

Created attachment 345048 [details] First pass

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.

(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 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.

(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.

Created attachment 345113 [details] Address feedback.

Created attachment 345114 [details] Address feedback.

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 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 ^)

Created attachment 345155 [details] Patch for EWS

Created attachment 345167 [details] Patch for landing

Comment on attachment 345167 [details] Patch for landing Clearing flags on attachment: 345167 Committed r233891: <https://trac.webkit.org/changeset/233891>