ContentParent::GetAll() can get a dead instance

RESOLVED FIXED in Firefox 49

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cyu, Assigned: cyu)

Tracking

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48+ wontfix, firefox49+ fixed, firefox50+ fixed)

Details

Attachments

(1 attachment)

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]
Posted patch The fixSplinter Review
Attachment #8766195 - Flags: review?(wmccloskey)
Comment on attachment 8766195 [details] [diff] [review]
The fix

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

Thanks for catching this!
Attachment #8766195 - Flags: review?(wmccloskey) → review+
We should get this on aurora+beta when bug 1274516 lands on those branches.
Cervantes, can you do the request for aurora+beta approval?

Thanks for fixing this!
Flags: needinfo?(cyu)
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 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.
Flags: needinfo?(cyu)
Attachment #8766195 - Flags: approval-mozilla-beta?
Attachment #8766195 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8836868dd1e5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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 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.
Attachment #8766195 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
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.
cyu can you help us get these patches in shape to land on aurora? Thanks!
Flags: needinfo?(cyu)
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.
Flags: needinfo?(cyu)
Comment on attachment 8766195 [details] [diff] [review]
The fix

Probable fix for some content process crashes, let's bring this to aurora 49!
Attachment #8766195 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(cyu)
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.
Flags: needinfo?(cyu)
You need to log in before you can comment on or make changes to this bug.