[webkit-reviews] review denied: [Bug 182920] Null-dereference of the second argument `resource` of DocumentLoader::scheduleSubstituteResourceLoad : [Attachment 334257] Patch

[bugzilla-daemon at webkit.org]

Fujii Hironori <Hironori.Fujii at sony.com> has denied Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 182920: Null-dereference of the second argument `resource` of
DocumentLoader::scheduleSubstituteResourceLoad
https://bugs.webkit.org/show_bug.cgi?id=182920

Attachment 334257: Patch

https://bugs.webkit.org/attachment.cgi?id=334257&action=review




--- Comment #15 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 334257
  --> https://bugs.webkit.org/attachment.cgi?id=334257
Patch

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

Thank you for the review, Michael.

>> Source/WebCore/loader/DocumentLoader.cpp:1432
>>	deliverSubstituteResourcesAfterDelay();
> 
> I don't understand; why would you want to add nullptr to
m_pendingSubstituteResources? Isn't it better to avoid calling
scheduleSubstituteResourceLoad entirely in this case? Then you can make
scheduleArchiveLoad return false, instead of true, to indicate failure.
> 
> Does deliverSubstituteResourcesAfterDelay need to be called when the second
argument is nullptr? Probably not, I guess, so it should probably be OK to skip
the call to scheduleSubstituteResourceLoad entirely, right?

Yes, DocumentLoader::scheduleSubstituteResourceLoad is tricky, there are a
FIXME comment in DocumentLoader::substituteResourceDeliveryTimerFired.

https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/loader/DocumentLoad
er.cpp?rev=228852#L1384

>  // A null resource means that we should fail the load.
>  // FIXME: Maybe we should use another error here - something like "not in
cache".
>  loader->didFail(loader->cannotShowURLError());

I will submit another less tricky patch.