982298 - crash in mozilla::ScrollFrameHelper::ScrollToImpl(nsPoint, nsRect const&, nsIAtom*)

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Nick Thomas [:nthomas] (UTC+12)]

This bug was filed from the Socorro interface and is 
report bp-aa847442-e4dc-4fe5-a276-68abd2140311.
=============================================================

One way to hit this is:
* start editing a wiki.m.o page
* use cmd-f to search for a word
* click the 'Show preview' button, or 'Page' in the header
* search again with cmd-g (nothing happens)
* search again with cmd-g (crashes)

More crash reports:
bp-30c48d6f-c897-4a81-917c-92cb52140311
bp-b624898f-b8fc-4215-bcab-4a0712140311
bp-5a7d8cc7-51c9-47c6-9ba9-79ea12140311
bp-040f652d-cc0a-48ba-9754-20c042140311
bp-467c2a07-193c-40d3-bef0-3de852140311

Comment 1

[Nick Thomas [:nthomas] (UTC+12)]

I'm using Nightly on mac, crashing in todays and yesterdays builds, no information about further back.

Comment 2

[Daniel Holbert [:dholbert]]

Could you provide more specific instructions? (a specific wiki page to edit & specific word to search for)

I just tried to reproduce with https://wiki.mozilla.org/index.php?title=Platform&action=edit and the word "vidyo" (which occurs once on the page), and I didn't crash. (using latest Linux nightly, datestamp 2014-3-11)

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

I can't reproduce on that page, but this works:

1, load https://wiki.mozilla.org/index.php?title=Releases/Firefox_28.0/BuildNotes&action=edit
2, cmd-f to search for mock
3, click on 'Page' to switch back to actual page
4, cmd-g once to crash

Or use the 'Show Preview' button at 3 and cmd-g twice to crash.  Reloading with cmd-r at 3 doesn't crash.

Comment 4

[Mats Palmgren (inactive)]

With the STR in comment 3 I'm crashing on this line (on Linux):
http://hg.mozilla.org/mozilla-central/annotate/69fbe69ddb8c/layout/generic/nsGfxScrollFrame.cpp#l2089
which was added Feb 28, 2014 in bug 968647 part 1 (so I'm guessing it's a regression from that bug).

Comment 5

[Makoto Kato [:m_kato]]

docshell seems to be null

Comment 6

[Makoto Kato [:m_kato]]

Attachment: fix w/ test

Comment 7

[Makoto Kato [:m_kato]]

Comment on attachment 8392072 [details] [diff] [review]
fix w/ test

This is regression by bug 968647.  Type ahead find seems to cache old pres shell, so docshell is null.  Since any 3rd party addon may cache pres shell, I add null check in ScrollToImpl for more safety.

Comment 8

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07050b85efab

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/07050b85efab

Comment 10

[Paul Silaghi, QA [:pauly]]

Reproduced in Nightly 2014-03-11, Win 7 x64.
Verified fixed Nightly 31.0a1 (2014-03-19).

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8392072 [details] [diff] [review]
fix w/ test

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
bug 968647

User impact if declined: 
When user uses find menu, Firefox may crash after navigating to other page.

Testing completed (on m-c, etc.): 
landed in m-c.  And test case is comment #0. Also, I added mochitest for this.

Risk to taking this patch (and alternatives if risky): 
Added nullptr check only.  So low risk.

String or IDL/UUID changes made by this patch:
No

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b75e5179a9ef

Comment 13

[Ada [:adalucinet]]

Reproduced with Nightly from 2014-03-11 with STR from comment 3.
Verified as fixed on Firefox 30 beta 1 (Build Id: 20140428174145) on Mac OS X 10.8.5, Windows 7 x64 and Ubuntu 13.10 x32:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0

Comment 14

[Boris Zbarsky [:bzbarsky]]

Why does this test need Timer.jsm, exactly?  That causes it to try to clobber the default setTimeout/clearTimeout on the global...

Comment 15

[Makoto Kato [:m_kato]]

(In reply to Boris Zbarsky [:bz] from comment #14)
> Why does this test need Timer.jsm, exactly?  That causes it to try to
> clobber the default setTimeout/clearTimeout on the global...

Because I need timer.  Ah, I will change it to Components.utils.import("resource://gre/modules/Timer.jsm", xxxx);

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Because I need timer.

the test runs in a window.  So it has timers already...

Comment 17

[Makoto Kato [:m_kato]]

Attachment: Remove importing Timer.jsm

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8428447 [details] [diff] [review]
Remove importing Timer.jsm

r=me

Comment 19

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c51574053d6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c51574053d6