Summary: | Fix null dereferencing in ResourceTimingInformation::addResourceTiming | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, cdumez, commit-queue, japhet, ryanhaddad | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Alex Christensen
2016-06-01 11:38:57 PDT
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. |