Bug 162877 – Avoid null dereference when changing focus in design mode.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 290515 [details]
Patch

[Chris Dumez]

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?

[Brent Fulgham]

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.

[Chris Dumez]

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.

[Brent Fulgham]

Committed r206751: <http://trac.webkit.org/changeset/206751>

[Brent Fulgham]

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