Closed Bug 1455906 Opened 5 years ago Closed 5 years ago

webextension bookmark search fails with unknown error (dateadded not defined?)

Categories

(Firefox :: Bookmarks & History, defect, P2)

Product:
Firefox
Component:
Bookmarks & History
Platform:
x86_64
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: wagle, Assigned: lina)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

Bug 1455906 - Handle zero dates added and last modified times in Places maintenance.
59 bytes, text/x-review-board-request
standard8
: review+
Details 
My prototype quality webextension for listing all 12000 bookmarks in a fast scrollable region was working 2 weeks ago, and now no longer works for me.

The search call fails:

  Error: An unexpected error occurred  panel.js:39:5

in the debugger console, and

  result.dateAdded is undefined  ext-bookmarks.js:96

in the browser console.

This works fine for other people and other profiles.  The code that fails starts at:

  https://github.com/wagle/tagspace-tng/blob/master/sidebar/panel.js#L28

Having run out of ideas of what to check for, I asked on irc, and was strongly suggested that I submit a bug.

I don't know how you all would replicate the error.  I figure places.sqlite is somehow out of whack, but everything has checked out so far.  If you want, you can give me queries to place to it, or at least point me to the code in the source that fails to set dataAdded.
On irc :weaksauce pointed me to the even better debugger ("Developer Toolbox") which let me dig the single stepper into ext-bookmarks, where it blamed these entries in moz_bookmarks:

  "2","2",,"1","1","Bookmarks Menu",,,"0","1523937112569000","menu________"
  "3","2",,"1","2","Bookmarks Toolbar",,,"0","1523830490002000","toolbar_____"

that is, the dateAdded fields were 0 (or Null from 2004 or 2008?).  My hack that worked was to change those to:

  "2","2",,"1","1","Bookmarks Menu",,,"1","1520225467648000","menu________","0","1"
  "3","2",,"1","2","Bookmarks Toolbar",,,"1","1512333673247000","toolbar_____","0","1"

in a test copy.  What's the real fix?
To head off possible confusion, I'll note that SQLiteManager wants to quote integers for some reason when generating CSV.
Those are not supposed to be zero, our code creates them with a positive value afaict. The only way I may think of currently for that to happen is something else writing to places.sqlite without going through an official API.
did you ever insert or update into moz_bookmarks manually?
btw, the correct value is a (timestamp * 1000)
The "0" timestamps for those two dates back to at least Aug 15  2015 (my earliest easily accessible backup of places.sqlite).  With more effort, I can find earlier than that if you need.  The profile's earliest timestamps are from 2008 apparently, so I thought maybe they'd been like that since before the earliest user_version on the schema, and the upgrade sequence failed at some point.

In any case, in 2015, I was still severely allergic to messing up my profile by hacking on places without knowing what I was doing.  So I doubt it was a runaway user (me).  But I have a vague old memory about doing something to those two bookmarks to clear up some corruption.  I will now see if I left any notes of that.
interesting, we may want to add a maintenance task to fix missing dateAdded/lastModified values, they may come from a time where we didn't set dateAdded and could affect old profiles
Blocks: 1410877
Priority: -- → P2
Whiteboard: [fxsearch]
(In reply to Marco Bonardo [::mak] (Away 23 Apr - 1 May) from comment #6)
> interesting, we may want to add a maintenance task to fix missing
> dateAdded/lastModified values, they may come from a time where we didn't set
> dateAdded and could affect old profiles

Whoops, I pushed a patch just as you posted this comment, haha! We have a trigger to reset NULL dates added and last modified for Sync, but not zero...seems pretty straightforward to cover that case, though.
The dateadded fields are null-not-allowed, so I guess 0 is as close to NULL as the field can get?
(In reply to Perry Wagle from comment #9)
> The dateadded fields are null-not-allowed, so I guess 0 is as close to NULL
> as the field can get?

I don't think we have a `NOT NULL` constraint on them, unfortunately. :-( https://searchfox.org/mozilla-central/rev/fcd5cb3515d0e06592bba42e378f1b9518497e3d/toolkit/components/places/nsPlacesTables.h#96-97
I'm .. um .. "certain" something told me that the dateAdded field was set to not null, but I seem to be confused.  Neither my

Many apologies.
.. Neither my 2015 or current places.sqlite has it marked as such ..
Comment on attachment 8970247 [details]
Bug 1455906 - Handle zero dates added and last modified times in Places maintenance.

https://reviewboard.mozilla.org/r/239044/#review245318

Looks good, thanks.

When I tried applying locally, there was a little bitrot, you might get away with it when pushing, but you might need to update for PlacesDBUtils.jsm
Attachment #8970247 - Flags: review?(standard8) → review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc070a920308
Handle zero dates added and last modified times in Places maintenance. r=standard8
https://hg.mozilla.org/mozilla-central/rev/fc070a920308
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Assignee: nobody → kit
Dunno if this is a REOPEN or not, but I just now can't migrate from esr52 to nightly or developers due to the dateAdded bug.

I should be able to workaround it, but EoL for ESR52 is immanent, and others might have trouble.

From the debugger console:
result.dateAdded is undefined    ext-bookmarks.js:88
convertBookmark                  chrome://browser/content/parent/ext-bookmarks.js:88:5
map                              self-hosted:292:17
getAPI/search/<                  chrome://browser/content/parent/ext-bookmarks.js:222:69
Flags: needinfo?(lina)
I worked around it.
(In reply to Perry Wagle from comment #19)
> Dunno if this is a REOPEN or not, but I just now can't migrate from esr52 to
> nightly or developers due to the dateAdded bug.

You should potentially force run the maintenance task after upgrading - go to about:support (aka Help -> Troubleshooting Information), go to the Places Database section towards the bottom, and click on "Verify Integrity".

Normally the maintenance task will run in the background once a day, but only at times when Firefox is seen to be "idle" (I don't know the exact definition).
Flags: needinfo?(lina)
You need to log in before you can comment on or make changes to this bug.