705509 - Crash in mozilla::places::Database::GetAsyncStatement close to startup

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Scoobidiver (away)]

It's a startup crash that first appeared in 10.0a1/20111029.

Signature	mozilla::places::Database::GetAsyncStatement(nsACString_internal const&)
UUID	94325132-1d93-4c56-94bc-26a4b2111126
Date Processed	2011-11-26 09:53:08.210077
Uptime	3
Last Crash	21 seconds before submission
Install Age	2.8 hours since version was first installed.
Install Time	2011-11-26 15:04:54
Product	Firefox
Version	11.0a1
Build ID	20111126031027
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 23 stepping 10
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x8a
App Notes 	AdapterVendorID: 1002, AdapterDeviceID: 954f, AdapterSubsysID: 00000000, AdapterDriverVersion: 8.911.0.0
Processor Notes 	WARNING: JSON file missing Add-ons
EMCheckCompatibility	False

Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	mozilla::places::Database::GetAsyncStatement 	toolkit/components/places/Database.h:234
1 	xul.dll 	nsNavHistory::invalidateFrecencies 	toolkit/components/places/nsNavHistory.cpp:1265
2 	xul.dll 	mozilla::places::Database::MigrateV7Up 	toolkit/components/places/Database.cpp:981
3 	xul.dll 	mozilla::places::Database::InitSchema 	

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Aplaces%3A%3ADatabase%3A%3AGetAsyncStatement%28nsACString_internal%20const%26%29

Comment 1

[Marco Bonardo [:mak]]

looking at the stack, sounds like an upgrade from Firefox 3.0.x, may be reproduceable that way.

Comment 2

[Marco Bonardo [:mak]]

Taking the bug to investigate it.

Comment 3

[Marco Bonardo [:mak]]

I can't reproduce the bug upgrading from 3.0, but I think I nailed i down and it may be due to an upgrade from a 3.0 alpha version, before frecency was added to the schema, since we hit this code path:

  nsCOMPtr<mozIStorageStatement> hasFrecencyStatement;
  rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
      "SELECT frecency FROM moz_places"),
    getter_AddRefs(hasFrecencyStatement));

  if (NS_FAILED(rv)) {
    ...
    nsNavHistory* history = nsNavHistory::GetHistoryService();

And that causes a re-entrancy.
I'm looking around if I can figure out which alpha and find a still valid download link.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.0

I would like to see this in Aurora, since even if it may affect just a few users, debugging their problems in future may be hellish.

For that reason I added some tests:
- Check that upgrading a db with schema < 6 replaces the database
- Check that upgrading a db with a largely incomplete schema 6 replaces the database (at least the unique index on moz_places.url should exist)
- Check that upgrading a db with a schema 6 missing frecency does not crash (the actual crash in this bug)

Note that we may have some users out there coming from Firefox 3 alpha nightlies with broken indices, this is a long standing issue and at this point I'm not sure we can do much about that, they'll be fixed when we'll completely replace the database schema. This is mostly due to lack of testing in the past, just to underline how important is that we test things.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 577416 [details] [diff] [review]
patch v1.0

Review of attachment 577416 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thanks!

Comment 6

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/360687ba014a

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 577416 [details] [diff] [review]
patch v1.0

I would like to take this on Aurora since users are hitting the crash, and looks like a couple regressions were introduced with recent migration code move (that is part of Firefox 10).
We should help users updating from ancient versions (like 3.0) rather than crashing while they are trying to do so.
Comes with 3 tests for various migration environments.

Comment 8

[Sheila Mooney]

Not sure high volume but it's a regression introduced in 10. It would be nice to take if the fix isn't risky.

Comment 9

[Marco Bonardo [:mak]]

Thank you. The risk is low since this touches migration of really old profiles (<3.5) and has tests for each change.
https://hg.mozilla.org/releases/mozilla-aurora/rev/53b1db0a0d4b

Comment 10

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/360687ba014a

Comment 11

[u279076]

Assuming this is reproducible with a 3.0a* build, is there some way QA can create a profile which will reproduce this bug, and ultimately verify the fix?

Comment 12

[Marco Bonardo [:mak]]

I think the test are enough to verify this, though they include a couple places.sqlite databases you can copy to a profile and then try to launch the app.

Off-hand I don't remember which alpha version was, should be something around alpha7, the problem is that schema migrations at that time were coalesced into existing functions, thus creating nice bugs.

Comment 13

[u279076]

We'll rely on the tests for this.