Closed Bug 1056018 Opened 5 years ago Closed 5 years ago

CPOW crash with PCookieServiceChild::SendGetCookieString in content process

Categories

(Core :: IPC, defect)

x86
Windows NT
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
e10s m3+ ---

People

(Reporter: jimm, Assigned: mrbkap)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 3 obsolete files)

https://crash-stats.mozilla.com/report/index/76e3f38d-b843-4b7d-9b41-cd99d2140819

Not sure where to file this, its cpow related and the process type is content. I suppose we'll see a lot of these and I'm guessing the offending out calls might be pretty random.

billm, can you provide any guidance here?

Note, found this because the signature shows up in crash stats matching crash bug 1021053 which was blocking omtc rollout.
Flags: needinfo?(wmccloskey)
tracking-e10s: --- → +
Once I get bug 1049879 finished, we can categorize the cookie message as having urgent priority. That will make it okay to send while doing CPOW stuff.
Flags: needinfo?(wmccloskey)
Assignee: nobody → wmccloskey
I offered to take this from billm.
Assignee: wmccloskey → mrbkap
My plan for this bug is to switch the cookie message to have urgent priority (just add prio(urgent) to the IPDL after |sync| or |async|). Changing the priority means that it can be processed out of order with respect to other messages, so we'll have to audit any related messages and possibly change their priority to urgent as well.

Blake, it would be great if you could take care of the IME messages at the same time. I was planning to switch them all to be urgent as well. That should cover most of the IPC crashes we're seeing.
Attached patch Cookie fixes (obsolete) — Splinter Review
Cookies are delicious delicacies!
Attachment #8513053 - Flags: review?(wmccloskey)
Attached patch IME patch (obsolete) — Splinter Review
Attachment #8513054 - Flags: review?(wmccloskey)
Comment on attachment 8513053 [details] [diff] [review]
Cookie fixes

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

::: dom/base/test/chrome/cpows_parent.xul
@@ +65,4 @@
>        let obj = new data.ctor();
>        ok(obj.a === 3, "constructor call");
>        ok(document.title === "Hello, Kitty", "document node");
> +      is(typeof document.title, "string", "can get document.title");

I don't really understand how this is testing cookie code, but I guess Gecko can do strange things sometimes. Can you put a comment here to describe why we want to test this?
Attachment #8513053 - Flags: review?(wmccloskey) → review+
Attachment #8513054 - Flags: review?(wmccloskey) → review+
Attached patch cookies2 (obsolete) — Splinter Review
The problem is that, in response to the CPOW, we were sending up both a PCookieService constructor (async) and a GetCookieString (sync). Only the second one was actually asserting since async messages during CPOWs are okay.

When we converted GetCookieString to urgent priority, the assertion went away, but then GetCookieService was bypassing the PCookieService constructor and arriving at the parent first. The parent didn't know what to do with the GetCookieService message at that point (since it hadn't heard of the receiver ID) so it killed the child and returned an error.

The fix is to make the PCookieService constructor urgent as well. This is an unfortunate hazard of urgent messages--they can lead to more urgent messages. In this case, though, I think the trail ends. PNecko is managed by PContent, and that is guaranteed to exist already if we start one of these transactions.
Attachment #8513675 - Flags: review?(mrbkap)
Comment on attachment 8513675 [details] [diff] [review]
cookies2

Sorry for not realizing this beforehand.
Attachment #8513675 - Flags: review?(mrbkap) → review+
Attached patch Fix for orangeSplinter Review
This fix was suggested by billm over IRC. His hypothesis was that we were touching the browser before the contentDocument had been set up (and we were therefore using a small document shim). This patch makes the document shim just complete enough for this new case. This was green on try, so I'm going to go ahead and push it.
Attachment #8513053 - Attachment is obsolete: true
Attachment #8513054 - Attachment is obsolete: true
Attachment #8513675 - Attachment is obsolete: true
Attachment #8514642 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f41055343fd7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Duplicate of this bug: 1010737
FYI, this is still occurring - 

http://www.mathies.com/mozilla/bug1116884.txt

KillHard child signature breakdown:
-----------------------------------
118	62.4%	mozilla::net::PCookieServiceChild::SendGetCookieString(mozilla::ipc::URIParams const &,bool const &,bool const &,IPC::SerializedLoadContext const &,nsCString *)


IPC error breakdown per KillHard signature:
-------------------------------------------
mozilla::net::PCookieServiceChild::SendGetCookieString(mozilla::ipc::URIParams const &,bool const &,bool const &,IPC::SerializedLoadContext const &,nsCString *):
  119 - '(msgtype=0x3C0001,name=???) Message not allowed: cannot be sent/recvd in this state'
You need to log in before you can comment on or make changes to this bug.