Bug 221097 – Null dereference in DocumentLoader::commitData()

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Julian Gonzalez]

e.g.

frame #0: WebCore`WebCore::DocumentParser::setDocumentWasLoadedAsPartOfNavigation()+0
frame #1: WebCore`WebCore::DocumentLoader::commitData(char const*, unsigned long)+0
frame #2: WebKit`WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int)+0
frame #3: WebCore`WebCore::DocumentLoader::commitLoad(char const*, int)+0
frame #4: WebCore`WebCore::CachedRawResource::notifyClientsDataWasReceived(char const*, unsigned int)+0
frame #5: WebCore`WebCore::CachedRawResource::updateBuffer(WebCore::SharedBuffer&)+0
frame #6: WebCore`WebCore::SubresourceLoader::didReceiveDataOrBuffer(char const*, int, WTF::RefPtr<WebCore::SharedBuffer, WTF::DumbPtrTraits<WebCore::SharedBuffer> >&&, long long, WebCore::DataPayloadType)+0
frame #7: WebCore`WebCore::SubresourceLoader::didReceiveBuffer(WTF::Ref<WebCore::SharedBuffer, WTF::DumbPtrTraits<WebCore::SharedBuffer> >&&, long long, WebCore::DataPayloadType)+0
frame #8: WebCore`auto WebCore::ResourceLoader::loadDataURL()::$_2::operator()<WTF::Optional<WebCore::DataURLDecoder::Result> >(WTF::Optional<WebCore::DataURLDecoder::Result>)::'lambda'()::operator()()+0
frame #9: WebCore`WTF::CompletionHandler<void ()>::operator()()+0
frame #10: WebCore`WebCore::SubresourceLoader::didReceiveResponsePolicy()+0
frame #11: WebCore`WebCore::DocumentLoader::responseReceived(WebCore::ResourceResponse const&, WTF::CompletionHandler<void ()>&&)::$_3::operator()(WebCore::PolicyAction, WebCore::PolicyCheckIdentifier)+0

<rdar://problem/66168788>

[Julian Gonzalez]

Created attachment 418665 [details]
Patch

[Julian Gonzalez]

New test here is failing on Windows due to console output - coming up with a new patch now.

[Julian Gonzalez]

Created attachment 418685 [details]
Patch

[Julian Gonzalez]

I have a theory for the failing layout tests - the second change here (to move the null check in WebFrameLoaderClient::committedLoad()) isn't quite right. Looking into testing a fix...

[Julian Gonzalez]

Created attachment 418763 [details]
Patch

[Darin Adler]

Comment on attachment 418763 [details]
Patch

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

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1151
> +    if (m_frame->coreFrame()->document() && m_frame->coreFrame()->document()->isMediaDocument())
>          loader->cancelMainResourceLoad(pluginWillHandleLoadError(loader->response()));

Even better way to write it:

    if (is<MediaDocument>(m_frame->coreFrame()->document()))

The template function has the null check built in.

[Julian Gonzalez]

(In reply to Darin Adler from comment #6)
> Comment on attachment 418763 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418763&action=review
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1151
> > +    if (m_frame->coreFrame()->document() && m_frame->coreFrame()->document()->isMediaDocument())
> >          loader->cancelMainResourceLoad(pluginWillHandleLoadError(loader->response()));
> 
> Even better way to write it:
> 
>     if (is<MediaDocument>(m_frame->coreFrame()->document()))
> 
> The template function has the null check built in.

Thanks! That sounds like a great idea, but trying to use it I'm running into a compiler error:

/<...>/OpenSource/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1150:12: error: use of undeclared identifier 'MediaDocument'; did you mean 'endOfDocument'?
    if (is<MediaDocument>(m_frame->coreFrame()->document()))

Which makes me think the MediaDocument type isn't exposed. Trying to include "MediaDocument.h" or <WebCore/MediaDocument.h> however, doesn't work from this file. Any suggestions?

[Darin Adler]

(In reply to Julian Gonzalez from comment #7)
> Trying to include
> "MediaDocument.h" or <WebCore/MediaDocument.h> however, doesn't work from
> this file. Any suggestions?

Including <WebCore/MediaDocument.h> is the right thing to do. If it doesn’t work in Cocoa builds, then presumably we just need to change MediaDocument.h to be exposed from WebCore to WebKit, which is easiest to do on a Mac. Need to change the header’s visibility from "project" to "private".

[Julian Gonzalez]

(In reply to Darin Adler from comment #8)
> (In reply to Julian Gonzalez from comment #7)
> > Trying to include
> > "MediaDocument.h" or <WebCore/MediaDocument.h> however, doesn't work from
> > this file. Any suggestions?
> 
> Including <WebCore/MediaDocument.h> is the right thing to do. If it doesn’t
> work in Cocoa builds, then presumably we just need to change MediaDocument.h
> to be exposed from WebCore to WebKit, which is easiest to do on a Mac. Need
> to change the header’s visibility from "project" to "private".

Thanks, that worked!

[Julian Gonzalez]

Created attachment 419070 [details]
Patch

[Julian Gonzalez]

(In reply to Julian Gonzalez from comment #9)
> (In reply to Darin Adler from comment #8)
> > (In reply to Julian Gonzalez from comment #7)
> > > Trying to include
> > > "MediaDocument.h" or <WebCore/MediaDocument.h> however, doesn't work from
> > > this file. Any suggestions?
> > 
> > Including <WebCore/MediaDocument.h> is the right thing to do. If it doesn’t
> > work in Cocoa builds, then presumably we just need to change MediaDocument.h
> > to be exposed from WebCore to WebKit, which is easiest to do on a Mac. Need
> > to change the header’s visibility from "project" to "private".
> 
> Thanks, that worked!

Well, not quite:

In file included from DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-30.cpp:2:
../../Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:80:10: fatal error: WebCore/MediaDocument.h: No such file or directory
   80 | #include <WebCore/MediaDocument.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~

GTK, WPE, WinCairo fail to build.
Is there another list I also have to change?

[Ryosuke Niwa]

(In reply to Julian Gonzalez from comment #11)
> (In reply to Julian Gonzalez from comment #9)
> > (In reply to Darin Adler from comment #8)
> > > (In reply to Julian Gonzalez from comment #7)
> > > > Trying to include
> > > > "MediaDocument.h" or <WebCore/MediaDocument.h> however, doesn't work from
> > > > this file. Any suggestions?
> > > 
> > > Including <WebCore/MediaDocument.h> is the right thing to do. If it doesn’t
> > > work in Cocoa builds, then presumably we just need to change MediaDocument.h
> > > to be exposed from WebCore to WebKit, which is easiest to do on a Mac. Need
> > > to change the header’s visibility from "project" to "private".
> > 
> > Thanks, that worked!
> 
> Well, not quite:
> 
> In file included from
> DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-30.cpp:2:
> ../../Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:80:10:
> fatal error: WebCore/MediaDocument.h: No such file or directory
>    80 | #include <WebCore/MediaDocument.h>
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~
> 
> GTK, WPE, WinCairo fail to build.
> Is there another list I also have to change?

Oh, we probably need to export that file. Add it to Source/WebCore/Headers.cmake.

[Julian Gonzalez]

Created attachment 419172 [details]
Patch

[EWS]

Committed r272396: <https://trac.webkit.org/changeset/272396>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419172 [details].