Crash under WebPage::boundaryEventOccurred

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

A nullptr check is needed in this method.

[Per Arne Vollan]

Created attachment 374384 [details]
Patch

[Per Arne Vollan]

rdar://problem/53189800

[Geoffrey Garen]

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.

[Per Arne Vollan]

Created attachment 374386 [details]
Patch

[Chris Dumez]

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

[Chris Dumez]

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.

[Per Arne Vollan]

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

[Chris Dumez]

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

[Brent Fulgham]

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

[Per Arne Vollan]

Created attachment 374411 [details]
Patch

[Per Arne Vollan]

Comment on attachment 374411 [details]
Patch

Thanks for reviewing!

[Brent Fulgham]

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?

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 374411 [details]:

storage/indexeddb/dont-wedge.html bug 199883 (authors: [e-mail], [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 374411 [details]
Patch

Clearing flags on attachment: 374411

Committed r247620: <https://trac.webkit.org/changeset/247620>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[chris fleizach]

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