Fix null dereferencing in ResourceTimingInformation::addResourceTiming
Created attachment 280253 [details] Patch
Comment on attachment 280253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280253&action=review How was this noticed? How does it reproduce? How can we test it? > Source/WebCore/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). > + > + * loader/ResourceTimingInformation.cpp: There's an expected WebCore/ChangeLog line missing here...!
Created attachment 280255 [details] Patch
Comment on attachment 280255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280255&action=review > Source/WebCore/loader/ResourceTimingInformation.cpp:46 > + auto initiatorIt = m_initiatorMap.find(resource); Considering it does nothing when document is null, shouldn't we update the parameter to be a Document& document and do a null check at the call site if needed?
Comment on attachment 280255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280255&action=review > Source/WebCore/ChangeLog:9 > + This would crash sometimes in http/tests/security/cross-frame-access-custom.html Given that it's a flaky crash, is it actually correct to fix it with a null check? What will the observable failure be when we bail out early?
Created attachment 280257 [details] Patch
(In reply to comment #5) > Comment on attachment 280255 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=280255&action=review > > > Source/WebCore/ChangeLog:9 > > + This would crash sometimes in http/tests/security/cross-frame-access-custom.html > > Given that it's a flaky crash, is it actually correct to fix it with a null > check? What will the observable failure be when we bail out early? If there's no document, then the performance entry simply won't be recorded. I think this is correct. Chris's suggestion makes this patch look and feel much better. We were really assuming CachedResourceLoader::document always returned non-null, which was a bad assumption. There are other similar checks in CachedResourceLoader.cpp.
Comment on attachment 280257 [details] Patch Seems reasonable.
Comment on attachment 280257 [details] Patch Clearing flags on attachment: 280257 Committed r201565: <http://trac.webkit.org/changeset/201565>
All reviewed patches have been landed. Closing bug.