Bug 7223 - Reproducible crash when tabbing to a frame that has not been loaded
Summary: Reproducible crash when tabbing to a frame that has not been loaded
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Beth Dakin
URL:
Keywords: EasyFix, HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2006-02-12 15:16 PST by Jussi Hagman
Modified: 2006-03-21 09:49 PST (History)
3 users (show)

See Also:


Attachments
Reduced testcase (2.15 KB, application/zip)
2006-02-12 15:19 PST, Jussi Hagman
no flags Details
Crash log (21.37 KB, text/plain)
2006-02-12 15:21 PST, Jussi Hagman
no flags Details
Proposed patch (1.24 KB, patch)
2006-02-15 02:27 PST, Jussi Hagman
darin: review-
Details | Formatted Diff | Diff
Fix crash, attempt to maintain tab focus (881 bytes, patch)
2006-03-20 14:05 PST, Beth Dakin
bdakin: review-
Details | Formatted Diff | Diff
Another fix that doesn't give focus to the unloaded frame (852 bytes, patch)
2006-03-20 14:15 PST, Beth Dakin
mjs: review+
Details | Formatted Diff | Diff
with layout test and DRT additions (7.75 KB, patch)
2006-03-20 23:03 PST, Beth Dakin
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jussi Hagman 2006-02-12 15:16:55 PST
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.
Comment 1 Jussi Hagman 2006-02-12 15:19:49 PST
Created attachment 6439 [details]
Reduced testcase 

Testcase in a .zip file.
Comment 2 Jussi Hagman 2006-02-12 15:21:06 PST
Created attachment 6440 [details]
Crash log
Comment 3 Darin Adler 2006-02-12 21:32:50 PST
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;
Comment 4 Jussi Hagman 2006-02-15 02:27:48 PST
Created attachment 6501 [details]
Proposed patch

The change suggested by Darin fixes the problem.
Comment 5 Joost de Valk (AlthA) 2006-02-15 02:34:28 PST
Reassigning to Jussi since he attached a patch
Comment 6 Darin Adler 2006-02-15 08:06:29 PST
Comment on attachment 6501 [details]
Proposed patch

What about a test? Can we make an automated test for this or not?
Comment 7 Justin Garcia 2006-02-16 00:59:13 PST
> 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.
Comment 8 Jussi Hagman 2006-02-16 11:12:32 PST
(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.
Comment 9 Darin Adler 2006-02-17 16:48:51 PST
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.
Comment 10 Jussi Hagman 2006-02-19 20:56:16 PST
I actually can't reproduce this any more.. It seems like some change in 10.4.5 has rendered this invisible. 
Comment 11 Alexey Proskuryakov 2006-02-19 21:48:37 PST
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.
Comment 12 Jussi Hagman 2006-02-20 10:01:43 PST
(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.

Comment 13 Alice Liu 2006-03-20 07:24:18 PST
<rdar://problem/4483860>
Comment 14 Beth Dakin 2006-03-20 14:05:42 PST
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...
Comment 15 Beth Dakin 2006-03-20 14:15:35 PST
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?
Comment 16 John Sullivan 2006-03-20 14:23:03 PST
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).
Comment 17 John Sullivan 2006-03-20 14:24:02 PST
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.
Comment 18 Beth Dakin 2006-03-20 14:25:35 PST
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!
Comment 19 Beth Dakin 2006-03-20 23:03:46 PST
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.
Comment 20 Maciej Stachowiak 2006-03-20 23:07:07 PST
Comment on attachment 7206 [details]
with layout test and DRT additions

r=me
Comment 21 Justin Garcia 2006-03-21 03:40:50 PST
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.
Comment 22 Beth Dakin 2006-03-21 09:49:09 PST
I didn't see Justin's comments and committed the patch, but I will commit again to decrease the time.