609592 - Win7 JumpList is freeing up resources too early for History

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Bug 598229 made queries async, but the final call to free() is sync, thus we end up killing this._builder before queries complete (first problem).
Plus the timing we fire the queries seems still wrong since closing the connection fails when the final query runs (second problem).
This is reproduce-able by just running a single b-c test on win7, you'll see a "this._builder is undefined" error plus a "unable to close connection" assertion.

Since changing all the shutdown code would add far more complication to a relatively simple code (And make it dependent on Places shutdown, that I want to avoid if possible), and since on shutdown we need to update only if history is empty, I'm attaching a small optimization that will just empty the lists if history is empty.
This solves the shutdown problem since we don't run anymore queries on shutdown.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.1

ehr, better, this is not async so I don't want to call commitBuild.

Comment 2

[Jim Mathies [:jimm]]

>+    // If history is empty, just clear the list.
>+    if (!PlacesUtils.history.hasHistoryEntries) {
>+      this._buildCustom(_getString("taskbar.frequent.label"), items);
>+      return;
>+    }

>+    // If history is empty, just clear the list.
>+    if (!PlacesUtils.history.hasHistoryEntries) {
>+      this._buildCustom(_getString("taskbar.recent.label"), items);
>+      return;
>+    }

I don't think we need the _buildCustom calls. All we want is a task list, the other two list titles shouldn't show up.

Comment 3

[Jim Mathies [:jimm]]

(In reply to comment #2)
> >+    // If history is empty, just clear the list.
> >+    if (!PlacesUtils.history.hasHistoryEntries) {
> >+      this._buildCustom(_getString("taskbar.frequent.label"), items);
> >+      return;
> >+    }
> 
> >+    // If history is empty, just clear the list.
> >+    if (!PlacesUtils.history.hasHistoryEntries) {
> >+      this._buildCustom(_getString("taskbar.recent.label"), items);
> >+      return;
> >+    }
> 
> I don't think we need the _buildCustom calls. All we want is a task list, the
> other two list titles shouldn't show up.

Doesn't look they would do much anyway...

http://mxr.mozilla.org/mozilla-central/source/browser/components/wintaskbar/WindowsJumpLists.jsm#341

Comment 4

[Marco Bonardo [:mak]]

ah honestly I thought they were calling in _builder. thus a simple early return would do it!

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.2

So, we start building a list, if we don't add a custom list to it, it won't just appear since every time we replace everything. This makes sense.

The new patch just early returns history lists creation if history is empty, plus fixing the comment from Gavin in the original patch, regarding the use of Object.keys from javascript 1.8.5

Comment 6

[Marco Bonardo [:mak]]

ehr, this is not blocking was marked like that because I cloned the bug, will need approval.

Comment 7

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/07988ddcfe84