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.
Created attachment 6439 [details] Reduced testcase Testcase in a .zip file.
Created attachment 6440 [details] Crash log
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;
Created attachment 6501 [details] Proposed patch The change suggested by Darin fixes the problem.
Reassigning to Jussi since he attached a patch
Comment on attachment 6501 [details] Proposed patch What about a test? Can we make an automated test for this or not?
> 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.
(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 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.
I actually can't reproduce this any more.. It seems like some change in 10.4.5 has rendered this invisible.
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.
(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.
<rdar://problem/4483860>
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...
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?
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 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 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!
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 on attachment 7206 [details] with layout test and DRT additions r=me
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.
I didn't see Justin's comments and committed the patch, but I will commit again to decrease the time.