Bug 158266 – Fix null dereferencing in ResourceTimingInformation::addResourceTiming

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Fix null dereferencing in ResourceTimingInformation::addResourceTiming

[Alex Christensen]

Created attachment 280253 [details]
Patch

[Brady Eidson]

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...!

[Alex Christensen]

Created attachment 280255 [details]
Patch

[Chris Dumez]

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?

[Alexey Proskuryakov]

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?

[Alex Christensen]

Created attachment 280257 [details]
Patch

[Alex Christensen]

(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.

[Chris Dumez]

Comment on attachment 280257 [details]
Patch

Seems reasonable.

[WebKit Commit Bot]

Comment on attachment 280257 [details]
Patch

Clearing flags on attachment: 280257

Committed r201565: <http://trac.webkit.org/changeset/201565>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.