1283017 - ContentParent::GetAll() can get a dead instance

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P1)

Description

[Cervantes Yu [:cyu] [:cervantes]]

DataStorage calls ContentParent::GetAll(), which should return instances that are alive, but it doesn't.

Thread 1 "firefox" hit Breakpoint 1, mozilla::DataStorage::<lambda(mozilla::dom::ContentParent*)>::operator() (aParent=0x7fffae2fb000, __closure=<synthetic pointer>) at /mnt/SSD/data/hg/mozilla-central/security/manager/ssl/DataStorage.cpp:593
593         item.type() = aType;
(gdb) list
588
589       RunOnAllContentParents([&](dom::ContentParent* aParent) {
590         DataStorageItem item;
591         item.key() = aKey;
592         item.value() = aValue;
593         item.type() = aType;
594         Unused << aParent->SendDataStoragePut(mFilename, item);
595       });
596
597       return NS_OK;
(gdb) p aParent
$6 = (mozilla::dom::ContentParent *) 0x7fffae2fb000
(gdb) p aParent->mIsAlive
$7 = false

And the browser's stdout keeps spewing out the following once a content process has ever crashed:
###!!! [Parent][MessageChannel] Error: (msgtype=0x46001C,name=PContent::Msg_DataStoragePut) Channel error: cannot send/recv


###!!! [Parent][MessageChannel] Error: (msgtype=0x46001C,name=PContent::Msg_DataStoragePut) Channel error: cannot send/recv

[Thread 0x7fff8e980700 (LWP 31108) exited]

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: The fix

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8766195 [details] [diff] [review]
The fix

Review of attachment 8766195 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for catching this!

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

We should get this on aurora+beta when bug 1274516 lands on those branches.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Cervantes, can you do the request for aurora+beta approval?

Thanks for fixing this!

Comment 5

[Cervantes Yu [:cyu] [:cervantes]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8836868dd1e569fb075c32c5152782f18bf253cc
Bug 1283017 - Fix ContentParent::GetAll() returning a dead ContentParent instance if it's the first in the list. r=billm

Comment 6

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8766195 [details] [diff] [review]
The fix

Approval Request Comment
[Feature/regressing bug #]: Bug 1274516
[User impact if declined]: The browser tries to access a delegate of the content process. Probably crash like bug 1280150.
[Describe test coverage new/current, TreeHerder]: Tested locally. Autotests on try pending.
[Risks and why]: Pretty low. This is a small change to fix a regression, and we carefully move the pointer along the list data structure using the condition and try not to introduce more regressions.
[String/UUID change made/needed]: None.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8836868dd1e5

Comment 8

[Gerry Chang [:gchang]]

Tracking this as this is introduced by bug 1274516. Bug 1274516 is marked as won't fix in 48 beta, so mark this bug as won't fix in 48 beta, too.

Comment 9

[Gerry Chang [:gchang]]

Comment on attachment 8766195 [details] [diff] [review]
The fix

Review of attachment 8766195 [details] [diff] [review]:
-----------------------------------------------------------------

This is marked as won't fix in 48 beta. Not take it in 48 beta.

Comment 10

[Cervantes Yu [:cyu] [:cervantes]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=363da26b4d50

Comment 11

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Bug 1274516 was backed out from aurora 49. We can come back to this for uplift once the patch from 1274516 lands successfully on aurora.

Comment 12

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

cyu can you help us get these patches in shape to land on aurora? Thanks!

Comment 13

[Cervantes Yu [:cyu] [:cervantes]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fedb5c04ea54

Comment 14

[Cervantes Yu [:cyu] [:cervantes]]

Hi Liz, 

https://hg.mozilla.org/mozilla-central/rev/8d51d4e1930f for bug 1274516 and the patch for this bug apply to the latest aurora without any hiccups. They also build successfully on my workstation.

If https://treeherder.mozilla.org/#/jobs?repo=try&revision=a06e32db9e30 passes on all platforms, we can land the patches on aurora.

Comment 15

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8766195 [details] [diff] [review]
The fix

Probable fix for some content process crashes, let's bring this to aurora 49!

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

The android bustage was unrelated to this push. Hopefully nothing was hiding behind that bustage.

Despite that, though, this patch isn't applying cleanly, with or without the patch from bug 1274516, which has been backed out of aurora for bustage and is no longer being uplifted.

Comment 17

[Cervantes Yu [:cyu] [:cervantes]]

Liz, this patch only fixes the bug introduced by bug 1274516 and nothing more. 

Since 1274516 is marked as fix-optional for aurora and not being uplifted, this patch should also be treated as an optional fix.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4f2339c77cfc