631055 - Latest Test Pilot - Evaluation of Proposed Security is killing Firefox

Status: RESOLVED FIXED

(Mozilla Labs Graveyard :: Test Pilot Studies, defect)

Description

[Adal]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10) Gecko/20100101 Firefox/4.0b10
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10) Gecko/20100101 Firefox/4.0b10

Firefox is very slow in the last hour since this test pilot started. I also get a lot of Script is not responding errors referring to "resource://testpilot/modules/experiment_data_store.js:153"

I can see other users reporting this:
http://input.mozilla.com/en-US/opinion/1292242
http://input.mozilla.com/en-US/opinion/1292204
http://input.mozilla.com/en-US/opinion/1292223
http://input.mozilla.com/en-US/opinion/1292194


Reproducible: Sometimes

Steps to Reproduce:
1. Accept Evaluation of Proposed Security Test Pilot
2. Use Firefox normally

Actual Results:  
The browser hangs for 1-10 seconds on about one page in three. Other times the script not responding error appears.

Comment 1

[Daniel C]

Every time I load a new page the hard drive thrashes.

What's the datastore for this? Is it SQLite? Why so much hard drive activity?

http://input.mozilla.com/en-US/beta/search?product=firefox&q=test+pilot

Please test more widely before deploying these studies.

This will put a lot of people off of testing the betas and they may lay the blame at Firefox's feet.

Comment 3

[Carlos Solís]

Confirmed on Firefox 4.0rc10 on Ubuntu Maverick. Checking the line referenced by the error window, the problem lies in the command " insStmt.finalize(); ". Developers of that command should be notified of this bug.

Comment 4

[Daniel C]

Not sure if this has anything to with SQLite or not. Just a heads up. I think
this will piss off a lot of people.

Comment 5

[Shawn Wilsher :sdwilsh]

What, exactly, does the latest study do?

Comment 6

[Jono Xia]

Study is taken down.

Comment 7

[Jono Xia]

While we investigate.

Comment 8

[Shawn Wilsher :sdwilsh]

Things I've noticed about this study:
1) It implements sha1 in JS, but probably wants to use nsICryptoHash
2) The regular expressions it's using are really complicated, and not cached.
3) There is some synchronous disk I/O here that needs to be async: https://hg.mozilla.org/labs/testpilotweb/file/afc9d76c7acb/testcases/cmu/strict_entry.js#l113 (see https://developer.mozilla.org/en/Storage#Executing_a_Statement and the big warning about synchronous disk I/O).
4) Statements don't appear to be finalized, although I assume _createStatement caches them in some way, but we need to make sure they are finalized also.

I don't see where it's adding to that database though, but I hope that is async too!

Comment 9

[Jono Xia]

I think it's the synchronous database read.  Aside from making it synchronous I will add an index to the column where it's looking for matches.

The part where it writes to the database is definitely async already.

This is my fault.  I didn't write this study but I did test and code review it.  Not well enough obviously.  The slowdown isn't noticeable on my machine and I overlooked the use of synchronous database API.

Comment 10

[Jono Xia]

To bring everybody up to date with what's happened since my last comment on this bug:

I worked with the study author to rapidly produce a revision (http://hg.mozilla.org/labs/testpilotweb/rev/b2164a4a9a06 and http://hg.mozilla.org/labs/testpilotweb/rev/9a7d75e92f36 ) that made the database reads async and also improved efficiency by drastically reducing the number of database writes done.  We pushed this out only to the group of users who were already running the first, performance-killing version of the study, and only for b10 users.  We allowed it to run to completion for those users while not deploying it to anyone new.

The study is over now; the data from that subset of users has been collected, and the study and has been taken down.

In response to this event the Test Pilot team had a post-mortem meeting and put together a new pre-release checklist that each study from now on will have to pass before it is released.  Ensuring that database operations are async and don't affect performance is a part of the new checklist.

I believe this bug can be closed now since that particular study is no longer an ongoing concern.  My apologies once again for not doing a better job on testing and code review before releasing this one.  I'll strive to do better in the future.

Comment 11

[Daniel C]

"INVALID" isn't really the right resolution (and a comment when resolving bugs as something other than "FIXED" is customary). This bug was a valid bug. I think FIXED would be appropriate here.