Bug 158266 - Fix null dereferencing in ResourceTimingInformation::addResourceTiming
Summary: Fix null dereferencing in ResourceTimingInformation::addResourceTiming
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-01 11:38 PDT by Alex Christensen
Modified: 2016-06-01 13:14 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.24 KB, patch)
2016-06-01 11:43 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (2.35 KB, patch)
2016-06-01 11:50 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (4.90 KB, patch)
2016-06-01 12:04 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2016-06-01 11:38:57 PDT
Fix null dereferencing in ResourceTimingInformation::addResourceTiming
Comment 1 Alex Christensen 2016-06-01 11:43:19 PDT
Created attachment 280253 [details]
Patch
Comment 2 Brady Eidson 2016-06-01 11:45:58 PDT
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...!
Comment 3 Alex Christensen 2016-06-01 11:50:14 PDT
Created attachment 280255 [details]
Patch
Comment 4 Chris Dumez 2016-06-01 11:53:36 PDT
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 5 Alexey Proskuryakov 2016-06-01 11:54:01 PDT
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?
Comment 6 Alex Christensen 2016-06-01 12:04:18 PDT
Created attachment 280257 [details]
Patch
Comment 7 Alex Christensen 2016-06-01 12:09:03 PDT
(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 8 Chris Dumez 2016-06-01 12:41:30 PDT
Comment on attachment 280257 [details]
Patch

Seems reasonable.
Comment 9 WebKit Commit Bot 2016-06-01 13:14:04 PDT
Comment on attachment 280257 [details]
Patch

Clearing flags on attachment: 280257

Committed r201565: <http://trac.webkit.org/changeset/201565>
Comment 10 WebKit Commit Bot 2016-06-01 13:14:09 PDT
All reviewed patches have been landed.  Closing bug.