Nice one, not the test fault.

Scenario: 
1. BrowserData asked to be removed due profile deletion.
2. As part of the routine call DOMStorageContextWrapper::GetSessionStorageUsage 
   happened.
3. It execute GetSessionStorageUsageHelper() via PostShutdownBlockingTask
4. Shutdown initiated, but as there shutdown blocking task in the task  queue it
   will be executed.
5. GetSessionStorageUsageHelper() will post task w/o aby shutdown blocking which
   contain some SessionStorageUsageInfo objects in binding state.
6. As there no any other shutdown blocking task at the task queue shutdown will
   be started.
7. Unexecuted tasks will be disposed, binding states will be destroyed.
8. Upon destruction of SessionStorageUsageInfo will be attempt to post shutdown
   blocking task, and it will trigger fail on DCHECK.

Suggested solution: 
Helper functions at the dom_storage_context_wrapper.cc should post tasks with 
shutdown blocking attribute.

Maybe we should get rid of PostShutdownBlockingTask() instead, since we can't rely on it blocking shutdown (e.g. android) anyhow?

I would eventually like to get to a world where we flush BLOCK_SHUTDOWN work on Android when the app receives a notification of being switched out/etc.

When everything has been migrated to TaskScheduler: everything can be flushed in one pass (no need to wind things down one thread at a time) and I'd like to make shutdown atomic (shutdown phase without winding down any threads than terminate process) -- perhaps we can one day dream of being able to go through that without even entering any kind of "shutdown" state.

All of that to say, I'm all for simplifying shutdown itself but I'd prefer we don't stop marking tasks BLOCK_SHUTDOWN when they should *ideally* block shutdown.

And this particular DCHECK is saying "hey you wanted this work to block shutdown but shutdown is already done... if it needs to block shutdown then you should have posted it earlier... and if it doesn't need to then don't post it as such.."

Looking at this closer, this is a new check which is catching a real DOMStorage bug.

The issue:
* ~DOMStorageContextImpl() attempts to post a block shutdown task to the BlockingPool to trigger ~SessionStorageDatabase which is ultimately intended to trigger ~DBImpl::~DBImpl() which is expected to "// Wait for background work to finish"

* but... it's possible that ~DOMStorageContextImpl() runs way too late (in this case when the SessionStorageUsageInfo which indirectly owns the last reference to it is dropped from the main MessageLoop's pending tasks in the last stages of shutdown [1]

[1] on shutdown: the main MessageLoop stops running tasks early in the shutdown phase, then BrowserMainLoop::ShutdownThreadsAndCleanUp() happens (after which posting a BLOCK_SHUTDOWN task to a pool will result in the check that happens in this bug), and at the very end the main MessageLoop is deleted (and brings the remaining tasks, posted to it too late, down with itself).


If DBImpl() really needs to block shutdown on it being flushed, it currently flakily doesn't =S...

This seems high priority to fix because it (1) triggers a DCHECK and (2) might result in corrupted DOMStorage DB or loss of data?

I think we can fix this by scheduling the required work in the DOMStorageContextImpl::Shutdown() method.

Probably, except that DOMStorageContextImpl::Shutdown() appears to be triggered from ~StoragePartitionImpl() which is potentially too late as well (appears to be ultimately hooked on BrowserContext which is also deleted late I think).

Essentially nothing should rely on tear down to flush IMO.

mek@, can you take a peek?

Has this actually caused any more test flakiness?

One thing I don't understand is the various comments here mentioning that destroying a SessionStorageUsageInfo object somehow triggers tasks being posted. It doesn't look like that struct contains now or has ever contained anything with non-trivial destructors that might do such a thing...

Anyway, if this isn't currently causing major test problems, I'd hesitate to spend much time fixing it, at least until dmurphs session storage mojofication efforts are done, as that's all going to change the shutdown code paths anyway.

There is definitely a bigger problem with it being hard to guarantee that data actually gets flushed to disk on shutdown, but I'm having trouble parsing the messages in this thread with what the code actually looks like/used to look like to figure out what the problem being discussed in this particular bug is...

I'm unlikely to be able to spend any time on this any time soon, so unassigning from myself.