Closed Bug 1283017 Opened 7 years ago Closed 7 years ago

ContentParent::GetAll() can get a dead instance


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




Tracking Status
firefox48 + wontfix
firefox49 + fixed
firefox50 + fixed


(Reporter: cyu, Assigned: cyu)




(1 file)

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
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       });
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]
Attached 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)
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?
Closed: 7 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, 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 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.
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.