Bug 142776 – Reproducible null deref under ScriptedAnimationController::createDisplayRefreshMonitor

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Reproducible null deref under ScriptedAnimationController::createDisplayRefreshMonitor

[Tim Horton]

Created attachment 248829 [details]
Patch

[Tim Horton]

<rdar://problem/18921338>

[Alexey Proskuryakov]

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.

[Darin Adler]

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.

[Tim Horton]

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

[Tim Horton]

http://trac.webkit.org/changeset/181656