Something is fishy with promiseDBConnection

VERIFIED FIXED in mozilla32



4 years ago
4 years ago


(Reporter: mano, Assigned: mano)


(Blocks: 1 bug, {regression})

Mac OS X
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)


(Whiteboard: p=5 s=it-32c-31a-30b.3 [qa-])


(1 attachment, 2 obsolete attachments)

See bug 1015629: My attempt to reuse promiseDBConnection (added in 988070) revealed that something is not right with promiseDBConnection. AsyncShutdown behaved as if the connection is still open on shutdown. Marco thinks we might be creating the connection too late. This has the potential of a hard-to-catch regression in bookmarks backups, so we should figure this out before the new backup code is delivered.

Since backups already used Sqlite.jsm async connection, there's only one notable difference I could think of: getBookmarksTree created the DB connection each time it was called, and closed it when it was done. the DB connection in promiseDBConnection, however, is shared between consumers and created lazily. When it's created, we set a shutdown listener for closing it. If, by any chance, the connection is created too late (that is, during the shutdown process), the shutdown listener may not be called at all, and the connection will remain open.
Flags: firefox-backlog+
This sounds like a good use for Sqlite.shutdown.addBlocker.
Created attachment 8429913 [details] [diff] [review]
Assignee: nobody → mano
Attachment #8429913 - Flags: review?(mak77)
Running try on Windows (all unit tests) for this patch along with the patch on bug 1015629.
Comment on attachment 8429913 [details] [diff] [review]

Review of attachment 8429913 [details] [diff] [review]:

There might still be a race condition if shutdown starts before the call to addBlocker, in which case the call to `addBlocker` will raise an exception. You should either make sure that the call to addBlocker takes place early or try/catch your call to `addBlocker` and close the db immediately in case of exception.
Problem is, the caller expects a connection. I guess we should close the connection as you suggested, but still throw the exception.
Actually, I should just return the promise of openConnection to the caller. How exactly the connection is closed doesn't matter to the caller.
Created attachment 8429991 [details] [diff] [review]

New try build with this patch (along with asyncGetBookmarkIds).
Attachment #8429913 - Attachment is obsolete: true
Attachment #8429913 - Flags: review?(mak77)
Attachment #8429991 - Flags: review?(mak77)
Created attachment 8429993 [details] [diff] [review]
The Right File
Attachment #8429991 - Attachment is obsolete: true
Attachment #8429991 - Flags: review?(mak77)
Attachment #8429993 - Flags: review?(mak77)
Marco, please add this to the current iteration.

Mano, can you provide an estimate?
There is still this failure

TEST-UNEXPECTED-FAIL : xperf: File '{profile}\places.sqlite-wal' was accessed and we were not expecting it. DiskReadCount: 4, DiskWriteCount: 0, DiskReadBytes: 6INFO : RSS: Main: 161124352
Return code: 1 

this is Talos xperf complaining something accessed a file during the test and that was not expected. This test has a whitelist here

the whitelist contains places.sqlite and places.sqlite-shm, it should also contain places.sqlite-wal, I'm not sure why it doesn't, keeping a second open connection here may make a difference. I suggest to file a dependency to fix xperf whitelist, get review from jmaher or someone from ateam, and then land both.
the maximum values read from various try respins are  DiskReadCount: 32, DiskWriteCount: 0, DiskReadBytes: 983824, DiskWriteBytes: 0
Added to Iteration 32.3
Whiteboard: p=0 s=it-32c-31a-30b.3 [qa?]


4 years ago
Depends on: 1017214
Comment on attachment 8429993 [details] [diff] [review]
The Right File

Review of attachment 8429993 [details] [diff] [review]:

to be landed once the xperf issue is fixed in talos and that code as been deployed to the builders (I don't recall if that's immediate or not)

::: toolkit/components/places/PlacesUtils.jsm
@@ +1912,5 @@
> +      // It's too late to block shutdown, just close the connection.
> +      return conn.close();
> +    }
> +    return Promise.resolve();
> +  }, Cu.reportError).then(null, Cu.reportError);

I think the first Cu.reportError is not needed, if you don't provide an handler the error should be catched by the last then() error handler.
Attachment #8429993 - Flags: review?(mak77) → review+
No QA can be done here. Marco and I will watch tests and telemetry data.
Whiteboard: p=0 s=it-32c-31a-30b.3 [qa?] → p=0 s=it-32c-31a-30b.3 [qa-]
Hi Mano, can you provide a point estimate.
Flags: needinfo?(mano)
Hey Marco,

Along with the blocking bug (bug 1017214), I would say medium 5.
Flags: needinfo?(mano)
The talos changes are in fx-team. No Armageddon reported so far, so I'm going to check this in.
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32


4 years ago
Whiteboard: p=0 s=it-32c-31a-30b.3 [qa-] → p=5 s=it-32c-31a-30b.3 [qa-]
You need to log in before you can comment on or make changes to this bug.