Bug 111784 – Inspector: activate/deactivate IndexedDB transactions when inspecting IDB

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Alec Flett]

Inspector: activate/deactivate IndexedDB transactions when inspecting IDB

[Alec Flett]

Created attachment 192098 [details]
Patch

[Alec Flett]

vsevik: This may not be an ideal place for this, but it works great.

abarth@ - any insight here? Basically we have V8RecursionScope (which requires ScriptExecutionContext) which does some IDB-specific work, in addition to delivering MutationObserver changes. 

CC'ing adamk@ in case he has any ideas here since I don't know what MutationObserver is supposed to do with inspector mutation changes and he may want that.

I tried to put the V8RecursionScope outside of InspectorIndexedDBAgent itself (it probably belongs somewhere around InspectorController::dispatchFromFrontEnd() but there's no easily-accessible ScriptExecutionContext at that level) and that means it has to go somewhere in the code generator for InspectorBackendDispatcherImpl::dispatch()

In addition, this adds additional V8-specific code to Inspector, also probably not ideal. (JSC has a similar problem across the codebase being addressed in bug 106732, but not for this particular issue in the inspector) we could put something in WebCore, but it would be Yet Another Scoping object. 

So a totally different alternative would be to replicate the one line of V8RecursionScope that we need:

    IDBPendingTransactionMonitor::deactivateNewTransactions();

inside InspectorIDBAgent - less v8 specific, but I'm concerned that any future changes to V8RecursionScope will be missed, and that does seem like the place that we're trying to deliver microtask-based events.

[Alec Flett]

hmm.. autocomplete. adding abarth@ - see my previous comment.

[Adam Barth]

Comment on attachment 192098 [details]
Patch

This seems odd.  I don't understand why this would be needed.  We're not spinning a nested event loop, are we?

[Adam Klein]

V8RecursionScope is just semantically wrong here: it's supposed to be counting script invocations (which is what defines a microtask); I take it from the fact that adding this scope was necessary that no script ran

 Note that MutationObservers have a separate mechanism for firing when no script has run: see http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/WebKit.cpp#L78

But that doesn't really help address your cross-platform problem, as other platforms haven't added support for this end-of-task hook (as evidenced by the fact that bug 78290 is still open).

[Alec Flett]

Comment on attachment 192098 [details]
Patch

yeah I'm now reasonably convinced that V8RecursionScope is the wrong thing here.

[Alec Flett]

Created attachment 192114 [details]
Patch

[Alec Flett]

ok, back to IDB-specific code. 

abarth@ - r?

[Adam Barth]

Comment on attachment 192114 [details]
Patch

I don't really understand what this patch is doing.  Should we have a test for this patch?

[Alec Flett]

abarth@ sorry. I can ask vsevik if there is a reasonable way to test this stuff.

Whats basically happening is that we're calling into IDB from C++, bypassing V8 altogether. IDB's transaction state management (part of the stuff that auto-commits after a microtask) relies on the transaction being inactive as IDB starts to process them. Normally this happens as soon as you leave v8:

basically:
- script (V8) calls into IDB API - objects must be in an 'active' state during the script.
- script finishes, tells IDB to deactivate objects
- IDB starts processing transactions, 'activating' them as necessary. 

That 3rd step has a lot of ASSERTions around making sure that the transactions really are inactive, otherwise something has gone horribly wrong.

This bug makes it so that the inspector can call into IDB and the behavior stays the same.

[Vsevolod Vlasov]

I assume this is the same issue as https://bugs.webkit.org/show_bug.cgi?id=101618 ?
If so, it is already (implicitly) covered with tests, we just need to adjust the expectations.

[Alec Flett]

Created attachment 192509 [details]
Patch

[Alec Flett]

abarth@ - And here is a patch with a Layout Test. I'm following the existing patterns in the inspector of firing an event (the inspector has its own event-dispatching system) 

The test now selects (see 'objectStoreElement.select()') the objectStore, which is the user interaction that will cause IndexedDB to fire an assertion.

I'm working on a whole series of features to the inspector, so I have actual, new tests coming soon.

[Alec Flett]

vsevik@ - I'd say this is kind of a dupe yes - but it is not implicitly covered by tests - database-data.html does not test this a best as I can tell.

*** This bug has been marked as a duplicate of bug 101618 ***