Bug 150315 – ERROR: Unhandled web process message 'StorageAreaMap:DispatchStorageEvent'

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

This happens a lot in the GTK+ debug bot. It was introduced in r184930, that keeps the session storage area maps alive in the UI process when they are destroyed by the web process. The problem is that we also keep the listeners, so that events are also dispatched to the listeners of destroyed areas, and the message handlers were removed in the web process.

[Carlos Garcia Campos]

Created attachment 263445 [details]
Patch

[Carlos Garcia Campos]

Created attachment 263455 [details]
Updated patch

Addressed all review comments.

[Carlos Garcia Campos]

Oops, wrong bug, sorry for the noise.

[Andreas Kling]

Comment on attachment 263445 [details]
Patch

r=me

[Carlos Garcia Campos]

Committed r191333: <http://trac.webkit.org/changeset/191333>

[Alexey Proskuryakov]

This caused multiple assertions on regression tests: https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r191333%20(482)/results.html

Will roll out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 150362

[Carlos Garcia Campos]

Yes, I didn't take into account that now we can have session storage maps without a listener when the connection is closed and destroyStorageMap() hasn't been called. We could either simply change the ASSERT to something like 

ASSERT(isSessionStporage() || m_eventListeners.contains(std::make_pair(&connection, storageMapID)));

or add checks to processDidCloseConnection() and applicationWillTerminate() in case of session storage to not call removeListener if the listener is not present. Since HashMap handles the case of removing a non exiting item, I think it's simpler and probably more efficient to change the assert. Although, it's true that it wouldn't catch valid cases of session storage maps that should have a listener at that point. What do you guys think?

[Carlos Garcia Campos]

Created attachment 263812 [details]
Changed the assert

Ok, took the simplest approach and changed the assert. Just cq+ this if you agree with changing the assert.

[Andreas Kling]

Comment on attachment 263812 [details]
Changed the assert

Sure, this seems fine.

[WebKit Commit Bot]

Comment on attachment 263812 [details]
Changed the assert

Clearing flags on attachment: 263812

Committed r191481: <http://trac.webkit.org/changeset/191481>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

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