Improve PCOMContentPermissionRequest usage safety

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dougt, Assigned: wchen)

Tracking

Trunk
mozilla17
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
From bug 761930 - jdm - 

Given that there are now so many implementations of PCOMContentPermissionRequest, I would like to propose some runtime asserts to help catch any misuses of the class. Specifically, add a member variable mIPCOpen that is initialized to true, and is set to false in a method that is called by TabChild::DeallocPContentPermissionRequest. Add a destructor to PCOMContentPermissionRequest that asserts that mIPCOpen is false, which should catch any cases when the request object dies before IPDL would expect it to.

Comment 1

5 years ago
We could even override PTabChild::SendPContentPermissionRequest to call a method that sets mIPCOpen to true, to ensure we don't have any problems with request objects that are created but not sent. Belt and suspenders, etc.
(Assignee)

Updated

5 years ago
Assignee: nobody → wchen
(Assignee)

Comment 2

5 years ago
Created attachment 643045 [details] [diff] [review]
Added runtime assertion to ensure PCOMContentPermissionRequestChild is live while protocol is live.
Attachment #643045 - Flags: review?(doug.turner)
(Reporter)

Updated

5 years ago
Attachment #643045 - Flags: review?(doug.turner) → review?(josh)

Comment 3

5 years ago
Comment on attachment 643045 [details] [diff] [review]
Added runtime assertion to ensure PCOMContentPermissionRequestChild is live while protocol is live.

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

::: dom/ipc/PCOMContentPermissionRequestChild.h
@@ +25,5 @@
> +  PCOMContentPermissionRequestChild() : mIPCOpen(false) {}
> +  virtual ~PCOMContentPermissionRequestChild() {
> +    // mIPCOpen is set to true in TabChild::SendPContentPermissionRequestConstructor
> +    // and set to false in TabChild::DeallocPContentPermissionRequest
> +    NS_ASSERTION(!mIPCOpen, "Protocol must not be open when PCOMContentPermissionRequestChild is destroyed.");

Let's make this MOZ_ASSERT.
Attachment #643045 - Flags: review?(josh) → review+
(Assignee)

Comment 4

5 years ago
Created attachment 643492 [details] [diff] [review]
Added runtime assertion to ensure PCOMContentPermissionRequestChild is live while protocol is live.

Changed NS_ASSERTION to MOZ_ASSERT
Attachment #643045 - Attachment is obsolete: true
Attachment #643492 - Flags: review?(josh)
(Assignee)

Comment 5

5 years ago
Created attachment 643494 [details] [diff] [review]
Added runtime assertion to ensure PCOMContentPermissionRequestChild is live while protocol is live.

Updated patch description with r=jdm
Attachment #643492 - Attachment is obsolete: true
Attachment #643492 - Flags: review?(josh)
Attachment #643494 - Flags: review?(josh)

Updated

5 years ago
Attachment #643494 - Flags: review?(josh) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/6528e7d3af05
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6528e7d3af05
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.