Bug 7223 – Reproducible crash when tabbing to a frame that has not been loaded

• Component: Frames
• Status: RESOLVED FIXED

[Jussi Hagman]

A reduction of this problem is included as an attachment.

Steps to reproduce:
1. Open frameCrash/index.html
2. Press option-tab to select the link on page. (repeat and/or change settings if the link is not highlighed)
3. Press option-tab once more to go to the other frame

Result:
Crash with both Nightly Webkit and Safari.

Crash report and the reduced example are provided as attachments.

[Jussi Hagman]

Created attachment 6439 [details]
Reduced testcase 

Testcase in a .zip file.

[Jussi Hagman]

Created attachment 6440 [details]
Crash log

[Darin Adler]

My guess is that this line of code:

            FrameView *childFrameWidget = widget->isFrameView() ? static_cast<FrameView *>(widget) : 0;

needs a check for widget of 0.

            FrameView *childFrameWidget = (widget && widget->isFrameView()) ? static_cast<FrameView *>(widget) : 0;

[Jussi Hagman]

Created attachment 6501 [details]
Proposed patch

The change suggested by Darin fixes the problem.

[Joost de Valk (AlthA)]

Reassigning to Jussi since he attached a patch

[Darin Adler]

Comment on attachment 6501 [details]
Proposed patch

What about a test? Can we make an automated test for this or not?

[Justin Garcia]

> What about a test? Can we make an automated test for this or not?

Jussie: you could add a keyDown method on the event sender and in it create an NSEvent and send it to DumpRenderTree's offscreen window.  It should be pretty straightforward to do and would let us convert a few of our manual tests into automated ones.

[Jussi Hagman]

(In reply to comment #7)
> > What about a test? Can we make an automated test for this or not?
> 
> Jussie: you could add a keyDown method on the event sender and in it create an
> NSEvent and send it to DumpRenderTree's offscreen window.  

I'll look into that and try doing it. Thanks for the pointer.

[Darin Adler]

Comment on attachment 6501 [details]
Proposed patch

I'm going to mark this review- since we want a test case. But if we decide that's too hard, lets get this reviewed and landed anyway. I'll be happy to review+ it when either there's a test case attached, or a story about why we gave up on trying to make one.

[Jussi Hagman]

I actually can't reproduce this any more.. It seems like some change in 10.4.5 has rendered this invisible. 

[Alexey Proskuryakov]

FWIW, I'm still on 10.4.4, and I can reproduce the problem in ToT. But I cannot reproduce it in stock Safari 2.0.3.

The patch doesn't work quite right for me. It eliminates the crash, but breaks the tab order for both Tab and Option+Tab - I can no longer cycle between the address bar, Google bar and the page - the focus gets stuck in the page.

[Jussi Hagman]

(In reply to comment #11)
> FWIW, I'm still on 10.4.4, and I can reproduce the problem in ToT. But I cannot
> reproduce it in stock Safari 2.0.3.

Interesting.

> The patch doesn't work quite right for me. It eliminates the crash, but breaks
> the tab order for both Tab and Option+Tab - I can no longer cycle between the
> address bar, Google bar and the page - the focus gets stuck in the page.

I can see that too now (was not looking good enough when I tried it). So this patch is broken. This is not the way to fix this :(

I'm reassigning this bug to the default assignee (if I'm allowed to do that) as I am not sure I can fix this and my spare time is going to be limited for a few weeks at least.

[Alice Liu]

<rdar://problem/4483860>

[Beth Dakin]

Created attachment 7198 [details]
Fix crash, attempt to maintain tab focus

Nil-checking the widget and calling focus on the node if we don't have a widget appear to fix the crash and the tab-focus issues. Although it does take two option-tabs to get from the "Crash" link to the address bar, but I think that's right because the focus is on the non-existant frame in the interim. I think that's the right behavior, but I'm not sure. Working on turning this into a layout test...

[Beth Dakin]

Created attachment 7199 [details]
Another fix that doesn't give focus to the unloaded frame

Here's a similar patch that just skips the unloaded frame in the event loop and passes the focus directly from the loaded frame to the address bar. Which is better behavior?

[John Sullivan]

From the UI point of view, there's no point in keyboard-focusing a node unless the keyboard is useful while that node is focused. If there's no contents, then you can't keyboard-scroll or anything like that, so skipping this node entirely seems like the right choice (patch #2).

[John Sullivan]

Comment on attachment 7199 [details]
Another fix that doesn't give focus to the unloaded frame

Fix seems fine, but I'll hold off on r=me until a layout test is available, or until you decide that no layout test is possible.

[Beth Dakin]

Comment on attachment 7198 [details]
Fix crash, attempt to maintain tab focus

In light of John's comments, I am going to r- the first patch and work on a layout test. Thanks John!

[Beth Dakin]

Created attachment 7206 [details]
with layout test and DRT additions

Here is the same patch adding mouseDown to dump render tree for a layout test.

[Maciej Stachowiak]

Comment on attachment 7206 [details]
with layout test and DRT additions

r=me

[Justin Garcia]

I would decrease the amount of time you wait to let the frame become fully constructed  100ms should do, it's what a few other tests use.

[Beth Dakin]

I didn't see Justin's comments and committed the patch, but I will commit again to decrease the time.