Bug 221097 - Null dereference in DocumentLoader::commitData()
Summary: Null dereference in DocumentLoader::commitData()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-28 13:10 PST by Julian Gonzalez
Modified: 2021-02-04 16:17 PST (History)
11 users (show)

See Also:


Attachments
Patch (6.07 KB, patch)
2021-01-28 13:18 PST, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (6.13 KB, patch)
2021-01-28 17:37 PST, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (5.79 KB, patch)
2021-01-29 13:11 PST, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (7.74 KB, patch)
2021-02-02 14:59 PST, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (8.30 KB, patch)
2021-02-03 12:34 PST, Julian Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Gonzalez 2021-01-28 13:10:07 PST
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>
Comment 1 Julian Gonzalez 2021-01-28 13:18:07 PST
Created attachment 418665 [details]
Patch
Comment 2 Julian Gonzalez 2021-01-28 15:35:04 PST
New test here is failing on Windows due to console output - coming up with a new patch now.
Comment 3 Julian Gonzalez 2021-01-28 17:37:56 PST
Created attachment 418685 [details]
Patch
Comment 4 Julian Gonzalez 2021-01-29 11:29:51 PST
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...
Comment 5 Julian Gonzalez 2021-01-29 13:11:52 PST
Created attachment 418763 [details]
Patch
Comment 6 Darin Adler 2021-02-01 16:28:10 PST
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.
Comment 7 Julian Gonzalez 2021-02-02 13:07:55 PST
(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?
Comment 8 Darin Adler 2021-02-02 13:16:28 PST
(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".
Comment 9 Julian Gonzalez 2021-02-02 14:58:59 PST
(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!
Comment 10 Julian Gonzalez 2021-02-02 14:59:16 PST
Created attachment 419070 [details]
Patch
Comment 11 Julian Gonzalez 2021-02-02 16:06:31 PST
(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?
Comment 12 Ryosuke Niwa 2021-02-02 18:13:15 PST
(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.
Comment 13 Julian Gonzalez 2021-02-03 12:34:48 PST
Created attachment 419172 [details]
Patch
Comment 14 EWS 2021-02-04 16:17:10 PST
Committed r272396: <https://trac.webkit.org/changeset/272396>

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