Summary: | Null dereference in DocumentLoader::commitData() | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julian Gonzalez <julian_a_gonzalez> | ||||||||||||
Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | achristensen, annulen, beidson, cdumez, darin, ews-watchlist, gyuyoung.kim, japhet, rniwa, ryuan.choi, sergio | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Julian Gonzalez
2021-01-28 13:10:07 PST
Created attachment 418665 [details]
Patch
New test here is failing on Windows due to console output - coming up with a new patch now. Created attachment 418685 [details]
Patch
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... Created attachment 418763 [details]
Patch
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. (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? (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". (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! Created attachment 419070 [details]
Patch
(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? (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. Created attachment 419172 [details]
Patch
Committed r272396: <https://trac.webkit.org/changeset/272396> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419172 [details]. |