372665 - Crash [@ PresShell::ScrollFrameIntoView] when focusing br during pageload

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, you need to download the testcase locally to get the crash, because of the use of enhanced privileges.

Talkback ID: TB29934294Z
0x00000000
PresShell::ScrollFrameIntoView  [mozilla/layout/base/nspresshell.cpp, line 4017]
nsGenericElement::SetFocus  [mozilla/content/base/src/nsgenericelement.cpp, line 2133]
nsEventStateManager::ShiftFocusInternal  [mozilla/content/events/src/nseventstatemanager.cpp, line 3356]
nsEventStateManager::ShiftFocus  [mozilla/content/events/src/nseventstatemanager.cpp, line 3193]
nsEventStateManager::PostHandleEvent  [mozilla/content/events/src/nseventstatemanager.cpp, line 2237]
0x026a5360
0x8b57f685

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: shows where the problem is

I might even propose this as a fix, because the method is
ScrollFrameIntoView(nsIFrame *aFrame, ...) - if aFrame gets deleted
before scrolling, the method should not, IMO, do anything.
Perhaps for other cases we need ScrollContentIntoView(nsIContent* aContent, ...)

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

But why does FlushPendingNotifications(Flush_OnlyReflow) delete anything...

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposed patch

Similar crash is mentioned also in https://bugzilla.mozilla.org/show_bug.cgi?id=303620#c7

See also http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLAnchorElement.cpp#243

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It looks to me like all callers would be OK with this becoming ScrollContentIntoView followed by applying this logic to the primary frame for the content. That would in fact simplify many callers.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(and of course we could hold a strong reference to the content and avoid using nsWeakFrame)

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 257385 [details] [diff] [review]
proposed patch

Ok, I'll make a new patch.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: ScrollContentIntoView

Maybe like this.
There are still some cases which need ScrollFrameIntoView and
and some cases where that just makes the code more readable, IMO.
But usually using ScrollContentIntoView makes code work more 
consistently.
I've also made presShell to be a strong ref in all cases where
ScrollContentIntoView is called.

If flushing removes presShell from document, that removes also
primaryframe and ScrollContentIntoView returns NS_ERROR_NULL_POINTER.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Who still needs to call ScrollFrameIntoView and why can't they call ScrollContentIntoView instead?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Some code in Accessibility, and some code in event listener manager
needs frames anyway, so I didn't want to change that.
Form elements use also (via layoututils) ScrollFrameIntoView.
Maybe a follow-up bug/patch to remove the rest of the uses of 
ScrollFrameIntoView - all those cases which aren't as trivial (and which 
need more testing) as in the current patch ?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 257453 [details] [diff] [review]
ScrollContentIntoView

Yeah, OK. Let's do that followup bug though.

Comment 11

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070324 Minefield/3.0a3pre

(In reply to comment #10)
> (From update of attachment 257453 [details] [diff] [review])
> Yeah, OK. Let's do that followup bug though.

Was there a follow-up bug filed? Should there a follow-up bug be filed?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> Was there a follow-up bug filed? Should there a follow-up bug be filed?
> 

bug 372797