[Chromium] http/tests/inspector/indexeddb/database-data.html ASSERT on Win7 following r133855

[Joshua Bell]

Flakiness dashboard:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Finspector%2Findexeddb%2Fdatabase-data.html

Changeset:

http://trac.webkit.org/changeset/133855

Assert:

STDERR: ASSERTION FAILED: active != m_active

(Stack is useless, unfortunately)

[Vsevolod Vlasov]

This is apparently ASSERT(active != m_active); in
void IDBTransaction::setActive(bool active)

Do you have an idea how can this happen?

[Joshua Bell]

(In reply to comment #1)
> This is apparently ASSERT(active != m_active); in
> void IDBTransaction::setActive(bool active)
> 
> Do you have an idea how can this happen?

Transactions are "active" when created (so requests can be filed against them), but go "inactive" when control returns to the event loop. When an event is later dispatched in IDBRequest::dispatchEvent the transaction is temporarily made active, the event is dispatched, then the transaction is made inactive again.

So the two likely causes are:
(1) recursion through IDBRequest::dispatchEvent (i.e. it's already active, and being made active again)
(2) the transaction is not being made inactive when control returns to the event loop

My guess would be #2, since it is currently done by having the script engine call into IDBPendingTransactionMonitor::deactivateNewTransactions() - e.g. in bindings/v8/V8RecursionScope.cpp

In the inspector, this could be done manually e.g. at the end of DataLoader::execute it could call idbTransaction->setActive(false). I'd do this by introducing a stack-allocated TransactionScope class with ~TransactionScope() { m_transaction->setActive(false); } and initialize the scope after the transaction is returned from transactionForDatabase().

[Vsevolod Vlasov]

This test is not failing any mroe and according to blamelist from the dashboard http://trac.webkit.org/log/?verbose=on&rev=134988&stop_rev=134516 this was likely fixed in https://bugs.webkit.org/show_bug.cgi?id=102450.
Josh, do you think the fix you mentioned above is still needed?

[Joshua Bell]

(In reply to comment #3)
> This test is not failing any mroe and according to blamelist from the dashboard http://trac.webkit.org/log/?verbose=on&rev=134988&stop_rev=134516 this was likely fixed in https://bugs.webkit.org/show_bug.cgi?id=102450.

Weird - I'm not sure how that would affect it, given that was a tiny tweak to a recent refactor.

> Josh, do you think the fix you mentioned above is still needed?

In theory, yes. But if it's not failing I wouldn't touch it for now.

[Robert Kroeger]

Fails on Win7 (release) and WinXP as well.

[Joshua Bell]

(In reply to comment #5)
> Fails on Win7 (release) and WinXP as well.

Yeah, looks like it's happening on Linux intermittently as well.

I still think my diagnosis is correct, maybe we want to land a simpler temp fix to ensure it resolves the issue i.e. call setActive() directly at the end of DataLoader::execute

[David Grogan]

This is reproducible manually as described in http://code.google.com/p/chromium/issues/detail?id=178882

Using the inspector to look at an IDB object store causes an assert to fail. I can reproduce by Resources->IndexedDB->Expand a database->Click an objectstore -> Aw snap

The assertion is ASSERTION FAILED: active != (m_state == Active) at IDBTransaction.cpp(205).
I added some debug logging at the failure point: m_state = 1, Active=1. So an active transaction is being set active.

Alec independently concluded that the solution was to call setActive() at the end of DataLoader::execute.

[Alec Flett]

*** Bug 111784 has been marked as a duplicate of this bug. ***

[Alec Flett]

Created attachment 192519 [details]
Patch

[Vsevolod Vlasov]

I don't get it: this bug is about layout test http/tests/inspector/indexeddb/database-data.html  failing on win debug. We assume this is caused by incorrect "active" state of the transaction triggered by inspector.

Your patch attempts to fix it, hence it should fix the test, meaning this behavior would be covered by this test.

The change in resources-panel.html test that you add here is not adding any new test coverage except for the inspector front-end as far as I can see, database-data.html test is already loading data form object store.

[Vsevolod Vlasov]

Comment on attachment 192519 [details]
Patch

r- per my comment above. I think InspectorIndexedDBAgent.cpp is all we really need here.

[Vsevolod Vlasov]

(In reply to comment #11)
> (From update of attachment 192519 [details])
> r- per my comment above. I think InspectorIndexedDBAgent.cpp is all we really need here.

Sprry, "I think InspectorIndexedDBAgent.cpp CHANGE is all we really need here."

[Alec Flett]

Created attachment 193609 [details]
Patch

[Alec Flett]

vsevik@ - I'm sorry you're right. I didn't realize there was a Crash expectation set for this test.

[Vsevolod Vlasov]

Comment on attachment 193609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193609&action=review

Please update ChangeLog before landing.

> Source/WebCore/ChangeLog:16
> +        * inspector/front-end/IndexedDBModel.js: Add new event type.

There is no any changes in inspector front-end anymore.

[Alec Flett]

Created attachment 193635 [details]
Patch for landing

[Alec Flett]

Created attachment 193637 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 193637 [details]
Patch for landing

Clearing flags on attachment: 193637

Committed r146116: <http://trac.webkit.org/changeset/146116>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.