1560211 - consider reopening env after creating store to work around crash in mdb_cursor_put

Status: RESOLVED INACTIVE

(Core :: SQLite and Embedded Database Bindings, enhancement, P3)

Description

[Myk Melez [:myk] [@mykmelez]]

Bug 1538541 seems to happen when we first open an environment, create a store in it, and then write a bunch of data to that store. I wonder if it's possible to work around the problem by reopening the environment and re-getting a handle to the store before writing data to it.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: reopen env after creating store

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a7f56cabe97
reopen env after creating store r=nanj,keeler

Comment 3

[Noemi Erli[:noemi_erli]]

Backed out changeset 0a7f56cabe97 (Bug 1560211) for failing in browser_startup_mainthreadio.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=252716376&resultStatus=testfailed%2Cbusted%2Cexception&revision=0a7f56cabe978bef9eafd5e6824761a49f268ae6

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=252716376&repo=autoland&lineNumber=3041

Backout: https://hg.mozilla.org/integration/autoland/rev/10a73d7fc462cb9b765a777055cff28cb16eb2fa

Comment 4

[Myk Melez [:myk] [@mykmelez]]

The changes reads XULStore's data.mdb file twice on startup, whereas currently the file is read once. That triggered the main thread file IO test. To address the issue, we can either whitelist the extra read—which is restricted to Nightly only until bug 1546838 is resolved—or wait to fix bug 1546838.

I'm going to opt for the latter for now, which makes this dependent on that bug. (It's also possible that we'll fix or work around bug 1538541 in the meantime, making this change unnecessary.)

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Could we land just the cert_storage changes in the meantime?

Comment 6

[Myk Melez [:myk] [@mykmelez]]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #5)

Could we land just the cert_storage changes in the meantime?

We could, but as I've been testing the change further, I've also come to doubt its efficacy.

Yesterday evening I adapted a program that Nan wrote into this LMDB test program written in C that resembles the way cert_storage and xulstore create and populate a database. I've been testing variations of it against the latest version of mdb.c in the Mozilla fork of LMDB, which asserts if mdb_page_search fails when called from mdb_cursor_init.

And it seems like mdb_page_search always fails the first time data is written to a named database, even if the environment is closed and reopened after the named database is created. If we ignore the mdb_page_search failure instead of asserting, then the data still gets written; but my theory is that something about that failure is responsible for the occasional crash in mdb_cursor_put; and closing/reopening the environment doesn't seem to avoid it.

So… I'm still open to landing the change, but I'm no longer optimistic about its chances. I'll continue debugging this afternoon and see if I can identify a better workaround.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Ah, ok. Well, my vote would be for investigating further, then. Thanks for the update!

Comment 8

[Victor Porof [:vporof][:vp]]

Closing in favor of bug 1594995.