Bug 142776 - Reproducible null deref under ScriptedAnimationController::createDisplayRefreshMonitor
Summary: Reproducible null deref under ScriptedAnimationController::createDisplayRefre...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-17 01:05 PDT by Tim Horton
Modified: 2015-03-17 12:17 PDT (History)
3 users (show)

See Also:


Attachments
Patch (18.68 KB, patch)
2015-03-17 01:10 PDT, Tim Horton
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2015-03-17 01:05:27 PDT
Reproducible null deref under ScriptedAnimationController::createDisplayRefreshMonitor
Comment 1 Tim Horton 2015-03-17 01:10:25 PDT
Created attachment 248829 [details]
Patch
Comment 2 Tim Horton 2015-03-17 01:11:00 PDT
<rdar://problem/18921338>
Comment 3 Alexey Proskuryakov 2015-03-17 06:40:09 PDT
Comment on attachment 248829 [details]
Patch

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

This looks like a lot of complexity to do the not quite right thing. Or did you determine that it's right to ignore the call?

> LayoutTests/fast/animation/request-animation-frame-unparented-iframe-crash.html:7
> +setTimeout(function () {

This races with parsing, as the timeout can fire before iframe is parsed, and certainly before it loads. 

I'd do this in onload.
Comment 4 Darin Adler 2015-03-17 08:10:00 PDT
Comment on attachment 248829 [details]
Patch

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

> Source/WebCore/dom/ScriptedAnimationController.cpp:232
> +    if (!m_document->page())
> +        return Optional<RefPtr<DisplayRefreshMonitor>>(nullptr);

This seems really subtle. The difference in semantic between an optional containing a nullptr and one containing Nullopt is not at all clear, so if we are going to use this idiom we’ll need a comment somewhere.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:44
> -    if (RefPtr<DisplayRefreshMonitor> monitor = client->createDisplayRefreshMonitor(displayID))
> -        return monitor.release();
> +    Optional<RefPtr<DisplayRefreshMonitor>> monitor = client->createDisplayRefreshMonitor(displayID);
> +    if (monitor)
> +        return monitor.value();

Why move the definition outside the if statement? Seems like it was better before.

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:97
>      DisplayRefreshMonitor* monitor = ensureMonitorForClient(client);

A function named “ensure” that can return null is not a normal pattern in WebKit code. We should fix that.
Comment 5 Tim Horton 2015-03-17 11:26:13 PDT
(In reply to comment #3)
> Comment on attachment 248829 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248829&action=review
> 
> This looks like a lot of complexity to do the not quite right thing. Or did
> you determine that it's right to ignore the call?

The other browsers agree (dropping the callback on the floor), so I'm going to go with it.

I'm much more concerned about creating the wrong kind of DisplayRefreshMonitor and then sticking it in the map and using it from somewhere else - that is very scary.

> > LayoutTests/fast/animation/request-animation-frame-unparented-iframe-crash.html:7
> > +setTimeout(function () {
> 
> This races with parsing, as the timeout can fire before iframe is parsed,
> and certainly before it loads. 
> 
> I'd do this in onload.

Of course! That was silly of me. Thanks!

(In reply to comment #4)
> Comment on attachment 248829 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248829&action=review
> 
> > Source/WebCore/dom/ScriptedAnimationController.cpp:232
> > +    if (!m_document->page())
> > +        return Optional<RefPtr<DisplayRefreshMonitor>>(nullptr);
> 
> This seems really subtle. The difference in semantic between an optional
> containing a nullptr and one containing Nullopt is not at all clear, so if
> we are going to use this idiom we’ll need a comment somewhere.

It is subtle. I'll leave a better comment.

I tried an alternative patch where ChromeClient *always* returns a value (never "I dunno, make the default") if it's available, moving the platform #ifs into ChromeClient. For some reason that exploded to be much messier than you'd expect (mostly I guess because currently the platform DisplayRefreshMonitor implementations are internal to WebCore).

> > Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:44
> > -    if (RefPtr<DisplayRefreshMonitor> monitor = client->createDisplayRefreshMonitor(displayID))
> > -        return monitor.release();
> > +    Optional<RefPtr<DisplayRefreshMonitor>> monitor = client->createDisplayRefreshMonitor(displayID);
> > +    if (monitor)
> > +        return monitor.value();
> 
> Why move the definition outside the if statement? Seems like it was better
> before.

Indeed, that was a mistake.

> > Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:97
> >      DisplayRefreshMonitor* monitor = ensureMonitorForClient(client);
> 
> A function named “ensure” that can return null is not a normal pattern in
> WebKit code. We should fix that.

tryEnsureMonitorForClient? createMonitorForClient? (creates can fail)
Comment 6 Tim Horton 2015-03-17 12:17:03 PDT
http://trac.webkit.org/changeset/181656