Bug 150315 - ERROR: Unhandled web process message 'StorageAreaMap:DispatchStorageEvent'
Summary: ERROR: Unhandled web process message 'StorageAreaMap:DispatchStorageEvent'
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 149387 (view as bug list)
Depends on: 150362
Blocks:
  Show dependency treegraph
 
Reported: 2015-10-18 23:27 PDT by Carlos Garcia Campos
Modified: 2015-10-24 13:05 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.31 KB, patch)
2015-10-18 23:31 PDT, Carlos Garcia Campos
kling: review+
Details | Formatted Diff | Diff
Updated patch (21.49 KB, patch)
2015-10-19 01:06 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Changed the assert (2.78 KB, patch)
2015-10-22 03:08 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2015-10-18 23:27:40 PDT
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.
Comment 1 Carlos Garcia Campos 2015-10-18 23:31:08 PDT
Created attachment 263445 [details]
Patch
Comment 2 Carlos Garcia Campos 2015-10-19 01:06:33 PDT
Created attachment 263455 [details]
Updated patch

Addressed all review comments.
Comment 3 Carlos Garcia Campos 2015-10-19 01:07:57 PDT
Oops, wrong bug, sorry for the noise.
Comment 4 Andreas Kling 2015-10-19 07:06:52 PDT
Comment on attachment 263445 [details]
Patch

r=me
Comment 5 Carlos Garcia Campos 2015-10-19 23:21:54 PDT
Committed r191333: <http://trac.webkit.org/changeset/191333>
Comment 6 Alexey Proskuryakov 2015-10-20 08:57:50 PDT
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.
Comment 7 WebKit Commit Bot 2015-10-20 08:59:43 PDT
Re-opened since this is blocked by bug 150362
Comment 8 Carlos Garcia Campos 2015-10-21 01:27:58 PDT
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?
Comment 9 Carlos Garcia Campos 2015-10-22 03:08:26 PDT
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.
Comment 10 Andreas Kling 2015-10-22 14:40:18 PDT
Comment on attachment 263812 [details]
Changed the assert

Sure, this seems fine.
Comment 11 WebKit Commit Bot 2015-10-22 15:25:57 PDT
Comment on attachment 263812 [details]
Changed the assert

Clearing flags on attachment: 263812

Committed r191481: <http://trac.webkit.org/changeset/191481>
Comment 12 WebKit Commit Bot 2015-10-22 15:26:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Michael Catanzaro 2015-10-24 13:05:42 PDT
*** Bug 149387 has been marked as a duplicate of this bug. ***