Bug 122496

Summary: Remove use of deprecatedDeleteAllValues in NPRemoteObjectMap::pluginDestroyed
Product: WebKit Reporter: Darin Adler <darin>
Component: WebKit2Assignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, commit-queue, gustavo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 122504, 122547    
Bug Blocks: 73757    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Darin Adler 2013-10-08 00:15:16 PDT 
Remove use of deleteAllValues in NPRemoteObjectMap::pluginDestroyed
Comment 1 Darin Adler 2013-10-08 00:17:01 PDT 
Created attachment 213668 [details]
Patch
Comment 2 Andreas Kling 2013-10-08 00:33:18 PDT 
Comment on attachment 213668 [details]
Patch

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

r=me, this is much nicer than the old code.

Thought: Do you think it would be cool to have a function that yanks items matching a [lambda] predicate out of a collection and returns them in a Vector?

> Source/WebKit2/ChangeLog:9
> +        (WebKit::NPRemoteObjectMap::registerNPObject): Don't call release when puttin objects

Typo, putting.

> Source/WebKit2/Shared/Plugins/NPRemoteObjectMap.cpp:200
> +    for (auto it = m_registeredNPObjects.begin(), end = m_registeredNPObjects.end(); it != end; ++it) {

You can use range-based for in WebKit2.

> Source/WebKit2/Shared/Plugins/NPRemoteObjectMap.cpp:209
> +    for (auto it = m_npObjectProxies.begin(), end = m_npObjectProxies.end(); it != end; ++it) {

Same here.
Comment 3 Darin Adler 2013-10-08 00:50:53 PDT 
(In reply to comment #2)
> Thought: Do you think it would be cool to have a function that yanks items matching a [lambda] predicate out of a collection and returns them in a Vector?

Probably. It would use the word “take” in its name.
Comment 4 Darin Adler 2013-10-08 00:57:01 PDT 
Committed r157090: <http://trac.webkit.org/changeset/157090>
Comment 5 Alexey Proskuryakov 2013-10-08 08:18:49 PDT 
This caused crashes all over the place in plug-in tests. Darin, are you around to take a look?
Comment 6 WebKit Commit Bot 2013-10-08 08:24:18 PDT 
Re-opened since this is blocked by bug 122504
Comment 7 Alexey Proskuryakov 2013-10-08 08:28:24 PDT 
Rolled out in <http://trac.webkit.org/changeset/157105>.

Test results: <http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r157090%20(12401)/results.html>. 

Unfortunately, there are no crash logs (might be that they are actually freezes incorrectly detected as crashes).
Comment 8 Darin Adler 2013-10-08 09:44:23 PDT 
Were the crashes only on Mountain Lion, or were they on Lion too?
Comment 9 Alexey Proskuryakov 2013-10-08 09:56:22 PDT 
Crashing on Lion too. Actually, that bot has crash logs - http://build.webkit.org/results/Apple%20Lion%20Debug%20WK2%20(Tests)/r157104%20(11972)/plugins/netscape-plugin-map-data-to-src-crash-log.txt
Comment 10 Darin Adler 2013-10-08 22:24:50 PDT 
Committed r157157: <http://trac.webkit.org/changeset/157157>
Comment 11 Gustavo Noronha (kov) 2013-10-09 05:09:29 PDT 
Looks like the crashes came back with the new commit =(
Comment 12 WebKit Commit Bot 2013-10-09 05:45:02 PDT 
Re-opened since this is blocked by bug 122547
Comment 14 Darin Adler 2013-10-09 09:39:22 PDT 
Really? I did lots of testing locally and saw no crashes. I guess I am testing this wrong. Anders, can you help?
Comment 15 Darin Adler 2013-10-09 09:41:05 PDT 
Created attachment 213786 [details]
Patch
Comment 16 Darin Adler 2013-10-09 09:41:38 PDT 
Anders, any ideas on why this second version of the patch still caused all those crashes on the bots, but not on my Mavericks development machine?
Comment 17 Darin Adler 2014-04-17 08:44:28 PDT 
Created attachment 229547 [details]
Patch
Comment 18 Darin Adler 2014-04-17 09:59:02 PDT 
Committed r167431: <http://trac.webkit.org/changeset/167431>