Closed Bug 982298 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox30 + verified
firefox31 --- verified

People

(Reporter: nthomas, Assigned: m_kato)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

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
I'm using Nightly on mac, crashing in todays and yesterdays builds, no information about further back.
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)
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.
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).
Blocks: 968647
Keywords: regression
OS: Mac OS X → All
docshell seems to be null
Assignee: nobody → m_kato
Attached patch fix w/ testSplinter Review
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.
Attachment #8392072 - Flags: review?(roc)
Reproduced in Nightly 2014-03-11, Win 7 x64.
Verified fixed Nightly 31.0a1 (2014-03-19).
Status: RESOLVED → VERIFIED
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
Attachment #8392072 - Flags: approval-mozilla-aurora?
Attachment #8392072 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 987227
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
Why does this test need Timer.jsm, exactly?  That causes it to try to clobber the default setTimeout/clearTimeout on the global...
Flags: needinfo?(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);
Flags: needinfo?(m_kato)
> Because I need timer.

the test runs in a window.  So it has timers already...
Attachment #8428447 - Flags: review?(bzbarsky)
Comment on attachment 8428447 [details] [diff] [review]
Remove importing Timer.jsm

r=me
Attachment #8428447 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.