Bug 162877 - Avoid null dereference when changing focus in design mode.
Summary: Avoid null dereference when changing focus in design mode.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-03 13:16 PDT by Brent Fulgham
Modified: 2016-10-03 14:39 PDT (History)
2 users (show)

See Also:


Attachments
Patch (4.21 KB, patch)
2016-10-03 13:51 PDT, Brent Fulgham
cdumez: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2016-10-03 13:16:13 PDT
A malformed web page consisting of nested iframes can trigger a null dereference when changing focus in design mode. In this scenario, the DOM Window's m_frame member is set to null. This null value is used without checking to refocus the document view on the now non-existant frame.
Comment 1 Brent Fulgham 2016-10-03 13:51:55 PDT
Created attachment 290515 [details]
Patch
Comment 2 Chris Dumez 2016-10-03 13:59:39 PDT
Comment on attachment 290515 [details]
Patch

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

r=me with comments on the test.

> LayoutTests/fast/frames/iframe-focus-crash.html:4
> +        if (window.testRunner) {

nit: unnecessary curly brackets.

> LayoutTests/fast/frames/resources/iframe-focus-crash.html:5
> +		    document.designMode='on';

The indentation is weird / wrong in this block.

> LayoutTests/fast/frames/resources/iframe-focus-crash.html:6
> +		    window.parent.setTimeout(function focusTest() {

We do not need to name this function.

> LayoutTests/fast/frames/resources/iframe-focus-crash.html:8
> +            }, 60);

Why 60, wouldn't 0 work?

> LayoutTests/fast/frames/resources/iframe-focus-crash.html:12
> +	    <iframe src='iframe-focus-crash.html'></iframe>

Eh, is this a recursive iframe? Could we do without the recursiveness?
Comment 3 Brent Fulgham 2016-10-03 14:18:11 PDT
Comment on attachment 290515 [details]
Patch

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

>> LayoutTests/fast/frames/resources/iframe-focus-crash.html:5
>> +		    document.designMode='on';
> 
> The indentation is weird / wrong in this block.

Strange. I wonder why webkit-patch didn't complain? It also seems fine in my text editor. I wonder if this is an artifact of the review tool?

>> LayoutTests/fast/frames/resources/iframe-focus-crash.html:6
>> +		    window.parent.setTimeout(function focusTest() {
> 
> We do not need to name this function.

OK!

>> LayoutTests/fast/frames/resources/iframe-focus-crash.html:8
>> +            }, 60);
> 
> Why 60, wouldn't 0 work?

Yep -- it will, and I'll change it.

>> LayoutTests/fast/frames/resources/iframe-focus-crash.html:12
>> +	    <iframe src='iframe-focus-crash.html'></iframe>
> 
> Eh, is this a recursive iframe? Could we do without the recursiveness?

Unfortunately it is needed to reproduce the crash.
Comment 4 Chris Dumez 2016-10-03 14:26:35 PDT
Comment on attachment 290515 [details]
Patch

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

>>> LayoutTests/fast/frames/resources/iframe-focus-crash.html:5
>>> +		    document.designMode='on';
>> 
>> The indentation is weird / wrong in this block.
> 
> Strange. I wonder why webkit-patch didn't complain? It also seems fine in my text editor. I wonder if this is an artifact of the review tool?

The reason is that you are mixing tabs and spaces. Please fix.
Comment 5 Brent Fulgham 2016-10-03 14:39:13 PDT
Committed r206751: <http://trac.webkit.org/changeset/206751>
Comment 6 Brent Fulgham 2016-10-03 14:39:24 PDT
(In reply to comment #4)
> > Strange. I wonder why webkit-patch didn't complain? It also seems fine in my text editor. I wonder if this is an artifact of the review tool?
> 
> The reason is that you are mixing tabs and spaces. Please fix.

Fixed.