ContentParent::GetAll() can get a dead instance

RESOLVED FIXED in Firefox 49

Status

()

P1
normal
RESOLVED FIXED
3 years ago
2 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]
(Assignee)

Comment 1

3 years ago
Created attachment 8766195 [details] [diff] [review]
The fix
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)
(Assignee)

Comment 5

3 years ago
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
(Assignee)

Comment 6

3 years ago
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?
status-firefox48: --- → affected
status-firefox49: --- → affected

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8836868dd1e5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox50: affected → fixed
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.
status-firefox48: affected → wontfix
tracking-firefox48: --- → +
tracking-firefox49: --- → +
tracking-firefox50: --- → +
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)
(Assignee)

Comment 14

3 years ago
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)
(Assignee)

Comment 17

2 years ago
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)

Comment 18

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/4f2339c77cfc
status-firefox49: affected → fixed
You need to log in before you can comment on or make changes to this bug.