Remove SQLITE_BUSY handling in transaction helper rollback, and correctly process SQLITE_ABORT_ROLLBACK for ROLLBACK(introduced by spec change in SQLite 3.7.11), because SQLite 3.7.17 was already used in Fx 24.0 at 2013/09 and 3.8.8.2 is currently used

NEW
Unassigned

Status

()

defect
P3
major
5 years ago
3 years ago

People

(Reporter: mak, Unassigned)

Tracking

(Blocks 1 bug)

24 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Spec change of SQLite is still not reflected to code of Storage])

Reporter

Description

5 years ago
We can remove the SQLITE_BUSY handling from rollback in the transaction helper, since "In older versions of SQLite the ROLLBACK will fail with an error code SQLITE_BUSY if there are any pending queries. In more recent versions of SQLite (since version 3.7.11 circa 2012-03-20) the ROLLBACK will proceed and pending statements will be aborted with an SQLITE_ABORT or SQLITE_ABORT_ROLLBACK error."
Current code has "potential infinite SQLITE_BUSY loop" if ROLLBACK/SQLITE_BUSY happens on "same set of rows of aTable" in multiple requests at same time.

   At Request #1                                                      At Request #2
   do {                                                                      do {
      rv = mConnection->ExecuteSimpleSQL(             rv = mConnection->ExecuteSimpleSQL(
           NS_LITERAL_CSTRING("ROLLBACK"));             NS_LITERAL_CSTRING("ROLLBACK"));
      if (rv == NS_ERROR_STORAGE_BUSY)               if (rv == NS_ERROR_STORAGE_BUSY)
        (void)PR_Sleep(PR_INTERVAL_NO_WAIT);        (void)PR_Sleep(PR_INTERVAL_NO_WAIT);
   } while (rv == NS_ERROR_STORAGE_BUSY) ;    } while (rv == NS_ERROR_STORAGE_BUSY);

1. When pending request exists, ROLLBACK is requested by  Request #1 and Request #2 at same time.
2. Because pending requests and other ROLLBACK request exists, SQLITE_BUSY is returned to both ROLLBACK requests.
3. mozStorageHelper requests PR_Sleep(PR_INTERVAL_NO_WAIT).
    Because of NO_WAIT, next ROLLBACK is immediately requested by both Requests at almost same time.
4. Goes to step 2
Because of PR_Sleep(PR_INTERVAL_NO_WAIT), mozStorageHelper may request ROLLBACK again immediately at both requests at same time, and SQLite may return SQLITE_BUSY always. I think "another ROLLBACK" is also a "pending request".

In a forum, following was reported:
   try{ localStorage.clear(); } catch(e){ alert(e); }
   Exception is  NS_ERROR_STORAGE_BUSY.
   This exception due to NS_ERROR_STORAGE_BUSY won't be cleared until restart of Firefox.
Because Table for localStorge is pretty simple, 
    CREATE TABLE webappsstore2 (scope TEXT, key TEXT, value TEXT, secure INTEGER, owner TEXT)
and because only "key"  column and "value" column is exposed to localStoraage user,
   API for user is getItem(aKey), setItem(aKey,aValue), removeItem(aKey), clear() only. 
   clear() == remove allikey/value of same "scope" for me. scope=URI of web page, and limited to own URL by "same origin policy".
   A localStorage user can update key/value of row for own "scope" only, and "scope" is hidden to user.
user can request "update of single row" only at same time except when clear().
Even though this charcteristics of localStorage, above "forever NS_ERROR_STORAGE_BUSY. in localStorage.clear()" acually occurred on localStorage.

I'm not sure ROLLBACK is issued by localStorage/mozStorage on webappsstore2, and "dead lock among clear() of multiple scopes" may occur. 
But I cannot imagine phenomenon other then "infinite SQLITE_BUSY loop due to multiple ROLLBACKs at same time" in this case, because user usually calls setItem(aKey)/removeItem(aKey) only for uodate of data and clear() is not so frequently used when localStorage.

When will SQLite version 3.7.11 or newer be used?
OS: Mac OS X → All
Hardware: x86 → All
(In reply to WADA from comment #1)
> When will SQLite version 3.7.11 or newer be used?

We're using 3.8.8.2 currently.
SQLITE_ABORT and SQLITE_ABORT_ROLLBACK
  https://www.sqlite.org/rescode.html#abort
  https://www.sqlite.org/rescode.html#abort_rollback
SQL As Understood By SQLite
  https://www.sqlite.org/lang_transaction.html
> In very old versions of SQLite (before version 3.7.11 circa 2012-03-20) 
> the ROLLBACK will fail with an error code SQLITE_BUSY if there are any pending queries.

What iis reason why still keeping code based on "very old versions of SQLite"?
(In reply to Andrew Sutherland [:asuth] from comment #2)
> > When will SQLite version 3.7.11 or newer be used?
> We're using 3.8.8.2 currently.

If so, some problems might have been caused by lack of SQLITE_ABORT_ROLLBACK handling in mozStoragePrivateHelpers?
  In MXR, SQLITE_ABORT_ROLLBACK is not seen in mozStoragePrivateHelpers related code.
  http://mxr.mozilla.org/mozilla-central/search?string=SQLITE_ABORT
  http://mxr.mozilla.org/mozilla-central/search?string=SQLITE_ABORT_ROLLBACK
  http://mxr.mozilla.org/mozilla-central/search?string=ABORT_ROLLBACK
Summary: Remove SQLITE_BUSY handling in transaction helper rollback → Remove SQLITE_BUSY handling in transaction helper rollback, and SQLITE_ABORT_ROLLBACK should be processed because SQLite 3.8.8.2 is already used
http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStoragePrivateHelpers.cpp#80
If  SQLITE_ABORT_ROLLBACK, NS_ERROR_FAILURE is passed to Rollback().
http://mxr.mozilla.org/mozilla-central/source/storage/public/mozStorageHelper.h#142
If  NS_ERROR_FAILURE is returned to Rollback(), .Rollback() does do nothing, and returns NS_ERROR_FAILURE to caller. 
Is NS_ERROR_FAILURE always processed correctly and appropriately by any caller of Rollback()?
(In addition to comment #5)
If localStorage(DOMStorage), following may occur in localStorage.clear() if SQLITE_ABORT_ROLLBACK happens.
   localStorage.clear().-> Create single transaction for row#A, row#B, row#C, ... -> get lock for row#A and modify
   -> try to aquire lock for row#B -> fails -> SQLITE_BUSY -> Transaction calls Rollback() because he can not call Commit() in this case
    -> if SQLITE_ABORT_ROLLBACK is returned to this ROLLBACK requeest, Rollback() returns ,NS_ERROR_FAILURE  to caller.
   Code when error is returned, what can code do is one like next.
            Rollback();
            if RC==OK, return RC==OK => Claller retries transaction because ROLLBACK is done
            else{ report error or write log; return RC==NS_ERROR_FAILURE; }
                => Caller has similar code, then Commit won't be happen, and Rollbck wont't be retried => lock for row#A won'rt be released
                => if next localStorage.clear() or setItem(row#A) is issued for same scope, SQLITE_BUSY is returned forever, until restart.
                A problem in Mozilla family is : "report error or write log" in above psuedo code == "if Debug build, write log" or "Does do nothing".

A "forever SQLITE_BUSY to localStorage.clear() which was reported at a forum" is perhaps phenomenon like above. 
In that case, Script coder fortunately(or unfortunately) used try/catch for localStorage.clear(), so he could be aware of error,.
     try{ localStorge.clear(); } catch(e){ alert("Clear failed"); }
then he changed to try{ localStorge.clear(); }catch(e){ alert("Error"); alert(e); }, so he could saw NS_ERROR_STORAGE_BUSY.
If call is update of single key(single row), SQLITE_BUSY occurs only on row#A for which lock is not released.
Even if clear() is used, if it's not put in try/catch block, user can not be aware of problem.
User can see funny phenomenon of "some rows somehow not updated as expected" only. But user usually won't be aware of problem, because error state is cleard by restart of Firefox.
Summary: Remove SQLITE_BUSY handling in transaction helper rollback, and SQLITE_ABORT_ROLLBACK should be processed because SQLite 3.8.8.2 is already used → Remove SQLITE_BUSY handling in transaction helper rollback, and SQLITE_ABORT_ROLLBACK should be correctly processed in Rollback() because SQLite 3.8.8.2 is already used
Severity: normal → major
Whiteboard: [Spec of SQLite is changed, but it's not reflected to code of Storage]
Summary: Remove SQLITE_BUSY handling in transaction helper rollback, and SQLITE_ABORT_ROLLBACK should be correctly processed in Rollback() because SQLite 3.8.8.2 is already used → Remove SQLITE_BUSY handling in transaction helper rollback, and SQLITE_ABORT_ROLLBACK(and SQLITE_ABORT too) should be correctly processed in Rollback() because SQLite 3.8.8.2 is already used
FYI.
An easy way to know SQLite version used by Firefox.
  Install "SQLite Manager" addon, Connect dabaseopen sqlite file), At "Execute SQL" Tab, SELECT sqlite_version(*), and "Run Now"
FYI.
SQLite version of trunk nightly build for Firefox esr 24.
Firefox esr 24.0
  Mozilla/5.0 (Windows NT 5.1; rv:24.0) Gecko/20130911 Firefox/24.0
  SELECT sqlite_version(*)
  3.7.17
Firefox esr 24.8.0esrpre
  Mozilla/5.0 (Windows NT 5.1; rv:24.0) Gecko/20140924 Firefox/24.0
  SELECT sqlite_version(*)
  3.7.17
In Firefox 24.0.0 which was released 2013/09, SQLite version was already upgraded to 3.7.17.
Summary: Remove SQLITE_BUSY handling in transaction helper rollback, and SQLITE_ABORT_ROLLBACK(and SQLITE_ABORT too) should be correctly processed in Rollback() because SQLite 3.8.8.2 is already used → Remove SQLITE_BUSY handling in transaction helper rollback, and SQLITE_ABORT_ROLLBACK/SQLITE_ABORT(by spec change in SQLite 3.7.11) should be correctly processed, because SQLite 3.7.17 was already used in Fx 24.0 at 2013/09 and 3.8.8.2 is currently used
Summary: Remove SQLITE_BUSY handling in transaction helper rollback, and SQLITE_ABORT_ROLLBACK/SQLITE_ABORT(by spec change in SQLite 3.7.11) should be correctly processed, because SQLite 3.7.17 was already used in Fx 24.0 at 2013/09 and 3.8.8.2 is currently used → Remove SQLITE_BUSY handling in transaction helper rollback, and correctly process SQLITE_ABORT_ROLLBACK/SQLITE_ABORT(by spec change in SQLite 3.7.11), because SQLite 3.7.17 was already used in Fx 24.0 at 2013/09 and 3.8.8.2 is currently used
Version: unspecified → 24 Branch
See Also: → 951934
FYI.
Following NSPR log with mozStorage:5 was reportd to a forum in Japan.
> 2015-03-06 05:44:35.927000 UTC - 3480[1b7fbef0]: sqlite3_trace on 22ea6d60 for 'INSERT OR REPLACE INTO webappsstore2 (scope, key, value) VALUES ('1.0.0.721.:http:8823', 'skin.gray2x', '{ ... }') '
> 2015-03-06 05:44:35.927000 UTC - 3480[1b7fbef0]: Statement::ExecuteStep error: database is locked
> 2015-03-06 05:44:35.927000 UTC - 3480[1b7fbef0]: sqlite3_trace on 22ea6d60 for 'ROLLBACK'

Because of localStorage, call is localStorage.setItem('skin.gray2x', ...).
It looks for me "SQLITE_BUSY to ROLLBACK request".
FYI.
Log of "Statement::ExecuteStep error:" is written when error is returned to following code.
http://mxr.mozilla.org/comm-central/source/mozilla/storage/src/mozStorageStatement.cpp#638
638 int srv = mDBConnection->stepStatement(mNativeConnection, mDBStatement);
639 
640 #ifdef PR_LOGGING
641   if (srv != SQLITE_ROW && srv != SQLITE_DONE) {
642       nsAutoCString errStr;
643       (void)mDBConnection->GetLastErrorString(errStr);
644       PR_LOG(gStorageLog, PR_LOG_DEBUG,
645              ("Statement::ExecuteStep error: %s", errStr.get()));
646   }
647 #endif
Whiteboard: [Spec of SQLite is changed, but it's not reflected to code of Storage] → [Spec change of SQLite is still not reflected to code of Storage]
No longer blocks: 1140826
Depends on: 1140826
I've opened bug 1140826, because SQLITE_ABORT is irrelevan to Rollbak() transaction helper and because SQLITE_ABORT_ROLLBACK issue is never transaction helper only issue.
Summary: Remove SQLITE_BUSY handling in transaction helper rollback, and correctly process SQLITE_ABORT_ROLLBACK/SQLITE_ABORT(by spec change in SQLite 3.7.11), because SQLite 3.7.17 was already used in Fx 24.0 at 2013/09 and 3.8.8.2 is currently used → Remove SQLITE_BUSY handling in transaction helper rollback, and correctly process SQLITE_ABORT_ROLLBACK for ROLLBACK(introduced by spec change in SQLite 3.7.11), because SQLite 3.7.17 was already used in Fx 24.0 at 2013/09 and 3.8.8.2 is currently used
Reporter

Updated

3 years ago
Priority: -- → P3
Blocks: 1140826
No longer depends on: 1140826
You need to log in before you can comment on or make changes to this bug.