Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Firefox full review feedback #257

Closed
gorhill opened this issue May 29, 2015 · 4 comments
Closed

Firefox full review feedback #257

gorhill opened this issue May 29, 2015 · 4 comments

Comments

@gorhill
Copy link
Owner

gorhill commented May 29, 2015

Opening a meta issue to keep track of the process and related code changes, etc.

All these changes will also need to be imported in uMatrix too, which I also have submitted for full review.

@gorhill
Copy link
Owner Author

gorhill commented May 29, 2015

2015-05-27:

  1. Synchronous SQL should not be used. The use of synchronous SQL via the storage system leads to severe responsiveness issues, and should be avoided at all costs. Please use asynchronous SQL via Sqlite.jsm (http://mzl.la/sqlite-jsm) or the executeAsync method, or otherwise switch to a simpler database such as JSON files or IndexedDB.
  2. SQL statements should be static strings. Dynamic SQL statement should be constucted via static strings, in combination with dynamic parameter binding via Sqlite.jsm wrappers (http://mzl.la/sqlite-jsm) or createAsyncStatement (https://developer.mozilla.org/en-US/docs/Storage#Binding_parameters)

Fixed with 53fc106 (0.9.8.1).

@gorhill
Copy link
Owner Author

gorhill commented May 29, 2015

2015-05-28:

  1. After installing I got in the Browser Console following error:
    NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageConnection.createStatement] vapi-background.js:234:0 (
    (chrome//ublock/content/js/vapi-background.js)

I could not reproduce, however I revised the code using API doc to harden SQLite code: c285ace (0.9.8.2, not submitted yet).

I tested the code against abnormal conditions by using restrictive r/w permissions on the sqlite file, and uBlock functioned properly (except for not being able to read and/or write settings of course).

@5t3f4n
Copy link

5t3f4n commented May 31, 2015

I see both uBlock Origin and uMatrix are up on AMO and at their respective latest versions.
Keep us updated on the review process.

Btw, do the GitHub versions now auto-update to the AMO ones? Or have you changed your mind on updating directly from GitHub?

@gorhill
Copy link
Owner Author

gorhill commented Jul 12, 2015

uBlock Origin was fully reviewed on July 4th, 2015.

@gorhill gorhill closed this as completed Jul 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants