820308 - [SeaMonkey] Test test_browserGlue_corrupt_nobackup.js times out

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Frank Wein [:mcsmurf]]

Since Sunday the test_browserGlue_corrupt_nobackup.js xpcshell test times out:
TEST-INFO | e:\builds\slave\test\build\xpcshell\tests\suite\common\places\tests\unit\test_browserGlue_corrupt.js | running test ...
TEST-PASS | e:\builds\slave\test\build\xpcshell\tests\suite\common\places\tests\unit\test_browserGlue_corrupt.js | test passed (time: 517.000ms)
TEST-INFO | e:\builds\slave\test\build\xpcshell\tests\suite\common\places\tests\unit\test_browserGlue_corrupt_nobackup.js | running test ...

command timed out: 1200 seconds without output, attempting to kill
SIGKILL failed to kill process

It looks like this test fail start with this comm-central revision:
http://hg.mozilla.org/comm-central/rev/2a546fb885e3
and this mozilla-central revision:
http://hg.mozilla.org/mozilla-central/rev/725eb8792d27

However, according to tbpl, this comm-central revision build fine with this m-c revision:
http://hg.mozilla.org/mozilla-central/rev/4e83d0987a31

According to http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4e83d0987a31&tochange=725eb8792d27 the first m-c revision (http://hg.mozilla.org/mozilla-central/rev/725eb8792d27) then must have to do the test fail.

Comment 1

[Frank Wein [:mcsmurf]]

Looks like the patch from Bug 763295.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

also the changes to browser/components/places/ should likely be ported.
Sorry, I forgot to file this bug earlier :(

Comment 3

[Frank Wein [:mcsmurf]]

Attachment: Patch

Marco: I'm not sure if you review SeaMonkey patches, if not, please tell. Then I'll refer to someone else. This is basically a one-to-one port of the Firefox patch (except for the migration thing, which is not part of SeaMonkey).

Comment 4

[Frank Wein [:mcsmurf]]

Comment on attachment 691527 [details] [diff] [review]
Patch

Something is wrong with this patch, the bookmarks export thing does not work as expected (sql assertion on shutdown, no bookmark file gets exported).

Comment 5

[Frank Wein [:mcsmurf]]

Marco: Just wondering, can you give me some advice where to start looking why the patch fails to export the HTML before shutdown? The places shutdown code in nsSuiteGlue.js looks identical to the Firefox nsBrowserGlue.js one.
I inserted a few dump(...) statements into my patch, the _shutdownPlaces function gets called as expected. Also the code flow enters this loop
        while (!shutdownComplete)
          thread.processNextEvents(true);

once. But after that loop runs once it fails to write out the bookmarks.html file (the onSuccess and onFailure functions don't get executed as I get no dump(...) output there). A debug build gives me this output:
WARNING: SQL statement 'SELECT h.id, h.url, IFNULL(b.title, h.title), h.rev_host
, h.visit_count, h.last_visit_date, f.url, null, b.id, b.dateAdded, b.lastModifi
ed, b.parent, null, h.frecency, b.position, b.type, b.fk, b.guid FROM moz_bookma
rks b LEFT JOIN moz_places h ON b.fk = h.id LEFT JOIN moz_favicons f ON h.favico
n_id = f.id WHERE b.parent = :parent ORDER BY b.position ASC' was not finalized:
 file c:/mozilla/tree-hg/comm-central/mozilla/storage/src/mozStorageConnection.c
pp, line 724
WARNING: SQL statement 'SELECT b.id, h.url, b.title, b.position, b.fk, b.parent,
 b.type, b.dateAdded, b.lastModified, b.guid, t.guid, t.parent FROM moz_bookmark
s b LEFT JOIN moz_bookmarks t ON t.id = b.parent LEFT JOIN moz_places h ON h.id
= b.fk WHERE b.id = :item_id' was not finalized: file c:/mozilla/tree-hg/comm-ce
ntral/mozilla/storage/src/mozStorageConnection.cpp, line 724
WARNING: SQL statement 'SELECT a.id, a.place_id, :anno_name, a.mime_type, a.cont
ent, a.flags, a.expiration, a.type FROM moz_anno_attributes n JOIN moz_annos a O
N n.id = a.anno_attribute_id JOIN moz_places h ON h.id = a.place_id WHERE h.url
= :page_url AND n.name = :anno_name' was not finalized: file c:/mozilla/tree-hg/
comm-central/mozilla/storage/src/mozStorageConnection.cpp, line 724
WARNING: SQL statement 'SELECT b.id, (SELECT id FROM moz_anno_attributes WHERE n
ame = :anno_name) AS nameid, a.id, a.dateAdded FROM moz_bookmarks b LEFT JOIN mo
z_items_annos a ON a.item_id = b.id AND a.anno_attribute_id = nameid WHERE b.id
= :item_id' was not finalized: file c:/mozilla/tree-hg/comm-central/mozilla/stor
age/src/mozStorageConnection.cpp, line 724
###!!! ASSERTION: sqlite3_close failed. There are probably outstanding statement
s that are listed above!: 'srv == SQLITE_OK', file c:/mozilla/tree-hg/comm-centr
al/mozilla/storage/src/mozStorageConnection.cpp, line 731
WARNING: nsExceptionService ignoring thread destruction after shutdown: file c:/
mozilla/tree-hg/comm-central/mozilla/xpcom/base/nsExceptionService.cpp, line 166

So it looks like places/sqlite shuts down before the export code can get the required information for bookmarks export? When I set browser.bookmarks.autoExportHTML to false, the assertion does not happen.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Frank Wein [:mcsmurf] from comment #5)
> So it looks like places/sqlite shuts down before the export code can get the
> required information for bookmarks export? When I set
> browser.bookmarks.autoExportHTML to false, the assertion does not happen.

does it work in Firefox? I don't exclude it may have broke :(

Comment 7

[Frank Wein [:mcsmurf]]

The pref works in Firefox, we talked about IRC on the issue itself.

Comment 8

[Frank Wein [:mcsmurf]]

Attachment: Patch

This one works, I had a stupid typo in the first patch (procesNextEvent_s_).

Comment 9

[[e-mail]]

Comment on attachment 692896 [details] [diff] [review]
Patch

>+          BookmarkHTMLUtils.importFromURL(bookmarksURI.spec, true).then(null,
>+            function onFailure() {
>+              Components.utils.reportError("Bookmarks.html file could be corrupt.");
>+            }
>+          ).then(
>+            function onComplete() {
>               // Ensure that smart bookmarks are created once the operation is
>               // complete.
>               this.ensurePlacesDefaultQueriesInitialized();
>+            }.bind(this)
>+          );
Sorry for the delay, I wasn't sure which version of Promise we were using (the devtools one I looked at first was broken, but Places actually uses the JetPack one which does the right thing here).

You don't need to write a function just to call a function, just pass the original function in the first place, i.e.

).then(this.ensurePlacesDefaultQueriesInitialized.bind(this));

(you might want to reformat that to keep the comment)

Comment 10

[[e-mail]]

Comment on attachment 692896 [details] [diff] [review]
Patch

>+        BookmarkHTMLUtils.exportToFile(FileUtils.getFile("BMarks", [])).then(
>+          function onSuccess() {
>+            shutdownComplete = true;
>+          },
>+          function onFailure() {
>+            // There is no point in reporting errors since we are shutting down.
>+            shutdownComplete = true;
>+          }
>+        );
Hmm, I wonder whether we can improve this by writing
BookmarkHTMLUtils.exportToFile(FileUtils.getFile("BMarks", [])).then().then(
  function onComplete() {
    shutdownComplete = true;
  }
);

Comment 11

[Frank Wein [:mcsmurf]]

Actually we need to fix something else for the test to pass (but I'm not yet sure what). Just writing down what I found out:
The test gets stuck here:
  onEndUpdateBatch: function() {
    let itemId = bs.getIdForItemAt(bs.toolbarFolder, 0);
    do_check_neq(itemId, -1);
    if (anno.itemHasAnnotation(itemId, "Places/SmartBookmark"))
      continue_test();

In our case the item has zero annotation names:
var annoNames = anno.getItemAnnotationNames(itemId);
do_print(annoNames.length);

Output:
TEST-INFO | c:/mozilla/tree-hg/objdirs/seamonkey-objdir-debug-tests/mozilla/_tes
ts/xpcshell/suite/common/places/tests/unit/test_browserGlue_corrupt_nobackup.js
| 0

Comment 12

[Frank Wein [:mcsmurf]]

Meh, ignore Comment 11. I made a mistake modifying the original patch.

Comment 13

[Frank Wein [:mcsmurf]]

Attachment: Patch for checkin

Includes fix from Comment 9, but not from Comment 10. I could not find out what the expected behavior of promise.then() actually is.

Comment 14

[Frank Wein [:mcsmurf]]

Pushed: https://hg.mozilla.org/comm-central/rev/68dcb9af22fb