Bug 199907

Created attachment 374384 [details] Patch

Comment on attachment 374384 [details] Patch Under what conditions can corePage() be null? I don't see anything that would ever clear it (except for the WebPage destructor). If the error here is use after free of WebPage, I think we need a different fix.

Created attachment 374386 [details] Patch

Comment on attachment 374384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374384&action=review > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6600 > + if (!corePage() || !corePage()->speechSynthesisClient()) I agree with Geoff, I do not see how corePage() can be null unless we're in the WebPage destructor. Can you also clarify how Page::speechSynthesisClient() can return null here? It never seems to get nulled out inside Page, and it seems to get initialized unconditionally (as long as ENABLED(SPEECH_SYNTHESIS)): #if ENABLE(SPEECH_SYNTHESIS) pageConfiguration.speechSynthesisClient = std::make_unique<WebSpeechSynthesisClient>(*this); #endif

Comment on attachment 374386 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374386&action=review > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6600 > + if (!corePage()->speechSynthesisClient()) Please see my comment on the previous patch.

(In reply to Chris Dumez from comment #6) > Comment on attachment 374386 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374386&action=review > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6600 > > + if (!corePage()->speechSynthesisClient()) > > Please see my comment on the previous patch. Yes, I believe you are right. The reason for the crash is probably that m_currentSpeechUtterance in SpeechSynthesis is null. I will update the patch. Thanks for reviewing, everybody!

(In reply to Per Arne Vollan from comment #7) > (In reply to Chris Dumez from comment #6) > > Comment on attachment 374386 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=374386&action=review > > > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6600 > > > + if (!corePage()->speechSynthesisClient()) > > > > Please see my comment on the previous patch. > > Yes, I believe you are right. The reason for the crash is probably that > m_currentSpeechUtterance in SpeechSynthesis is null. I will update the patch. This is what I said on the radar yesterday :) > > Thanks for reviewing, everybody!

Comment on attachment 374386 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374386&action=review >>>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6600 >>>> + if (!corePage()->speechSynthesisClient()) >>> >>> Please see my comment on the previous patch. >> >> Yes, I believe you are right. The reason for the crash is probably that m_currentSpeechUtterance in SpeechSynthesis is null. I will update the patch. >> >> Thanks for reviewing, everybody! > > This is what I said on the radar yesterday :) We do seem to have other cases where corePage() is nullptr checked, but perhaps those checks should not be present. Does the IPC layer not protect against return messages being sent to a WebContent process that is closing down? Maybe we are hitting the crash when something racy happens between the start of an async speech call, and the return message comes back when the process has terminated...

Created attachment 374411 [details] Patch

Comment on attachment 374411 [details] Patch Thanks for reviewing!

Comment on attachment 374411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374411&action=review > Source/WebCore/Modules/speech/SpeechSynthesis.cpp:255 > { I wonder if we hit this code when SpeechSynthesis::handleSpeakingCompleted() calls fireEvent with an error state?

The commit-queue encountered the following flaky tests while processing attachment 374411 [details]: storage/indexeddb/dont-wedge.html bug 199883 (authors: beidson@apple.com, commit-queue@webkit.org, dgrogan@chromium.org, and mark.lam@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 374411 [details] Patch Clearing flags on attachment: 374411 Committed r247620: <https://trac.webkit.org/changeset/247620>

All reviewed patches have been landed. Closing bug.

I'll add even with these changes, I'm hitting a lot of asserts and and debug asserts. working through it with rdar://53235469