472597 - history entries not being added in Fennec

Status: VERIFIED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

[Jono Xia]

Queries of the Places database which include selecting from "moz_places_temp" fail on Fennec (using alpha 2 on mac OS), but succeed on Shiretoko.  Calling tableExists("moz_places_temp") confirms that the table does not exist.  This table is supposed to be created in nsNavHistory::InitDB(), here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#886

so I don't understand how the table could fail to exist on Fennec.

This is currently blocking progress of the History sync feature in Weave for Fennec.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

I don't see any tables that should be created by nsNavHistory::InitTempTables and nsNavHistory::InitViews. The Fennec places.sqlite file has a schema version of 8 (PRAGMA user_version)

Comment 2

[Shawn Wilsher :sdwilsh]

That code runs always - regardless of the schema version.

Comment 3

[Dan Mills [:thunder]]

Requesting blocking-1.0, though I'd really like to know what's going on here much sooner as it breaks history sync on Fennec.

Comment 4

[Marco Bonardo [:mak]]

i really doubt TableExists can detect if the table exists since it makes

nsCString query("SELECT name FROM sqlite_master WHERE type = 'table' AND name ='");
while temp objects are retained in sqlite_temp_master

So that function will tell you that the table and other temp objects do not exist while they could be there.
That is a mozStorage bug, hwv won't change the fact queries are failing on your side.
To check for table existance do it manually
SELECT name FROM sqlite_temp_master WHERE type='table';

Comment 5

[Marco Bonardo [:mak]]

filed Bug 472963 against mozStorage

Comment 6

[Shawn Wilsher :sdwilsh]

So, if I were to come up with a C test case, could someone compile it with SQLite and run it on the n810 so I can try to make a reduced test case for an upstream ticket?  I /really/ don't see how this could be because of a bug in our code.

To be clear, mobile doesn't compile SQLite any differently than what we do in tree, right?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #6)
> So, if I were to come up with a C test case, could someone compile it with
> SQLite and run it on the n810 so I can try to make a reduced test case for an
> upstream ticket?

Yes

> 
> To be clear, mobile doesn't compile SQLite any differently than what we do in
> tree, right?

Not to my knowledge

Comment 8

[:Gavin Sharp [email: [e-mail]]]

For what it's worth, this isn't specific to the n810. I see it on a Mac and Windows Fennec builds as well. InitTempTables() is succeeding, but apparently the tables still aren't being created or something (side effect I'm seeing now are that pages aren't being marked visited, which causes bug 479984, since there are no "history" entries to show).

I don't see any relevant prefs that might not have defaults in storage code (only pref it appears to check is the "fsync behavior" one, and both Firefox and Fennec rely on the in-code default for that). Is Firefox doing something that Fennec isn't that's somehow causing the stuff done in InitTempTables() to take effect when it otherwise wouldn't?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Actually, scratch that - I think the moz_places_temp thing is a red herring. There's no reason to expect it to be in our places.sqlites (comment 1) given that it's a TEMP table, and a combination of Fennec not adding visits for some reason and comment 4 explain Jono's results in comment 0, I think.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Is this still a problem?

Comment 11

[Stuart Parmenter]

Jono?

Comment 12

[Jono Xia]

I will go take a look at the original code, undo my workaround, and see if I can still replicate the bug.  Stay tuned, folks...

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Weave uses a test like the one described in comment 4 to check for the missing moz_places_temp table:

http://hg.mozilla.org/labs/weave/file/89b98bf9e119/modules/engines/history.js#l164

I tested the same check in a current Fennec build and found the test passed - yes, there is now a moz_places_temp table. This means the check should pass for Weave too. In addition, "SELECT count(*) from moz_places_temp" succeeds and returns a large number.

I am marking this bug fixed or at least worksforme, since the actual fix happened in a different bug.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> I tested the same check in a current Fennec build and found the test passed -
> yes, there is now a moz_places_temp table. This means the check should pass for
> Weave too. In addition, "SELECT count(*) from moz_places_temp" succeeds and
> returns a large number.
I am a bit concerned that it's a "large number".  It shouldn't ever really get that large, and should be flushed to disk every two minutes (pref-able, so maybe you guys have changed it).

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #14)
> (In reply to comment #13)
> > I tested the same check in a current Fennec build and found the test passed -
> > yes, there is now a moz_places_temp table. This means the check should pass for
> > Weave too. In addition, "SELECT count(*) from moz_places_temp" succeeds and
> > returns a large number.
> I am a bit concerned that it's a "large number".  It shouldn't ever really get
> that large, and should be flushed to disk every two minutes (pref-able, so
> maybe you guys have changed it).

Excellent! "Large number" was 15 :)

Comment 16

[Shawn Wilsher :sdwilsh]

I retract my previous concern.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

sounds like this isn't an issue.