Last Comment Bug 773683 - Improve PCOMContentPermissionRequest usage safety
: Improve PCOMContentPermissionRequest usage safety
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: William Chen [:wchen] (Vacation until 8/25)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-13 09:33 PDT by Doug Turner (:dougt)
Modified: 2012-07-20 06:42 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Added runtime assertion to ensure PCOMContentPermissionRequestChild is live while protocol is live. (3.56 KB, patch)
2012-07-17 11:14 PDT, William Chen [:wchen] (Vacation until 8/25)
josh: review+
Details | Diff | Splinter Review
Added runtime assertion to ensure PCOMContentPermissionRequestChild is live while protocol is live. (3.56 KB, patch)
2012-07-18 12:07 PDT, William Chen [:wchen] (Vacation until 8/25)
no flags Details | Diff | Splinter Review
Added runtime assertion to ensure PCOMContentPermissionRequestChild is live while protocol is live. (3.57 KB, patch)
2012-07-18 12:10 PDT, William Chen [:wchen] (Vacation until 8/25)
josh: review+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2012-07-13 09:33:35 PDT
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 Josh Matthews [:jdm] 2012-07-13 09:37:43 PDT
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.
Comment 2 William Chen [:wchen] (Vacation until 8/25) 2012-07-17 11:14:07 PDT
Created attachment 643045 [details] [diff] [review]
Added runtime assertion to ensure PCOMContentPermissionRequestChild is live while protocol is live.
Comment 3 Josh Matthews [:jdm] 2012-07-17 14:16:13 PDT
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.
Comment 4 William Chen [:wchen] (Vacation until 8/25) 2012-07-18 12:07:53 PDT
Created attachment 643492 [details] [diff] [review]
Added runtime assertion to ensure PCOMContentPermissionRequestChild is live while protocol is live.

Changed NS_ASSERTION to MOZ_ASSERT
Comment 5 William Chen [:wchen] (Vacation until 8/25) 2012-07-18 12:10:46 PDT
Created attachment 643494 [details] [diff] [review]
Added runtime assertion to ensure PCOMContentPermissionRequestChild is live while protocol is live.

Updated patch description with r=jdm
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-07-19 19:09:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6528e7d3af05
Comment 7 Ed Morley [:emorley] 2012-07-20 06:42:51 PDT
https://hg.mozilla.org/mozilla-central/rev/6528e7d3af05

Note You need to log in before you can comment on or make changes to this bug.