264873 - [FIXr]onblur="window.close <-- pressing tab makes it crash [@ PresShell::ScrollFrameIntoView]

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Ed de Graaff]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910

guess the same as https://bugzilla.mozilla.org/show_bug.cgi?id=239563 , but on
tab press

Reproducible: Always
Steps to Reproduce:
1. open a window using window.open
2. make sure the popup contains OnBlur="javascript:window.close()"
3. press TAB having the popup focussed

Actual Results:  
mozilla exited, error occured

Expected Results:  
window close or stay, dont really matter

tried on 1.7.2, 1.7.3 and 2 other pc's

Comment 1

[Alan Plum]

Confirmed Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20041001
Firefox/0.10.1 StumbleUpon/1.998

Comment 2

[Martijn Wargers (dead)]

I crash with the URL testcase with Mozilla 1.7. Talkback ID: TB1373520M
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB1373520M

But I don't crash with:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041017
Firefox/0.9.1+

Comment 3

[Jeremy M. Dolan]

Happening on Aviary too. Confirming.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20041017 Firefox/1.0
Talkback submitted but talkback-public "quick search" seems totally broken, so I
can't get the ID.

You MUST load the pop-up as a pop-up. There is no crash if you load
http://home.hccnet.nl/de.graaff/popup.html
directly in the main window. Load
http://home.hccnet.nl/de.graaff/popupcaller.html
then manually open the pop-up from the "blocked pop-up" status or info bars.

Mousing in and out of the window calls the onblur, but there is no crash.

Comment 4

[Boris Zbarsky [:bzbarsky]]

The basic problem here is that the ShiftFocusInternal() code goes through, gets
the nextFocusFrame, then calls:

      SetContentState(nsnull, NS_EVENT_STATE_FOCUS);
      presShell->ScrollFrameIntoView(nextFocusFrame,
                                     NS_PRESSHELL_SCROLL_ANYWHERE,
                                     NS_PRESSHELL_SCROLL_ANYWHERE);

(this the the "if (sub_shell)" case).

The problem is that SetContentState fires focus/blur event handlers, which can
destroy nextFocusFrame (and for that matter, destroy presShell, since we're not
holding a strong ref to it).  So we're calling ScrollFrameIntoView on a deleted
destroyed frame, and end up crashing when we try to call
aFrame->GetContent()->GetDocument() because aFrame->mContnet is 0xdddddddd.

This code really should not be holding pointers to non-refcounted objects across
event dispatch.  In particular, it should reget nextFocusFrame after calling
SetContentState().  And it needs to hold a strong ref to the presshell.

Comment 5

[Aaron Leventhal]

In reply to comment #4)
>       SetContentState(nsnull, NS_EVENT_STATE_FOCUS);
>       presShell->ScrollFrameIntoView(nextFocusFrame,
>                                      NS_PRESSHELL_SCROLL_ANYWHERE,
>                                      NS_PRESSHELL_SCROLL_ANYWHERE);
> 
I think we should just try reversing the order of these two calls. I can't think
of any particular reason why the scrolling has to happen have to the
setcontentstate call.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to that effect

That makes sense to me...

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 163324 [details] [diff] [review]
Patch to that effect

Looks good to me too.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 163324 [details] [diff] [review]
Patch to that effect

Sure, looks fine.

I'd love to have a generic solution to the problem of holding weak pointers to
objects across event dispatch.	There are just too many places that this comes
up for us to find them all.  Maybe we could make all object destruction (dom
nodes, frames, presentation objects) be deleted on an event, after we've
unwound from the event handling code.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8b