Closed Bug 808983 Opened 12 years ago Closed 12 years ago

Should not call hal_sandbox::Vibrate when ipc is broken

Categories

(Core :: Hardware Abstraction Layer (HAL), defect)

Product:
Core
Component:
Hardware Abstraction Layer (HAL)
Platform:
x86_64
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19
Tracking Flags:
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: slee, Assigned: slee)

References

Details

Attachments

(1 file, 2 obsolete files)

patch v3
2.77 KB, patch
slee
: review+
bajaj
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
      No description provided.
Summary: Should not call hal_sandbox::Vibrate when child process is dead → Should not call hal_sandbox::Vibrate when ipc is broken
Attached patch patch (obsolete) — — Splinter Review 
Hi Justin,

This patch is to fix the problem that when the ipc is disconnected and content process still tries to call Vibrate/CancelVibrate. Would you please review it? If you don't have time, can you suggest someone to review it? BTW, could you suggest how to do auto test in this case?
Thanks.
Attachment #679070 - Flags: review?(justin.lebar+bug)
Comment on attachment 679070 [details] [diff] [review]
patch

This is good, but I'd like the comments to be clearer.  There's no longer an explicit call to hal_sandbox, so can you please say something like "Don't forward our ID if we're not in the sandbox, because hal_impl doesn't need it. ..." or something?
Attachment #679070 - Flags: review?(justin.lebar+bug) → feedback+
I'd like to have a look at the patch with updated comments, if you don't mind.
Attached patch patch v2 (obsolete) — — Splinter Review 
Justin,
Please check the comment of this patch. Thanks for your suggestion.
Attachment #679070 - Attachment is obsolete: true
Attachment #679516 - Flags: review?(justin.lebar+bug)
Comment on attachment 679516 [details] [diff] [review]
patch v2

> +  // Don't forward our ID if we are not in the sandbox, because hal_impl 
> +  // doesn't need it. And we don't want it to be tempted to read it.  The
> +  // empty identifier will assert if it's used.

r=me if you change "it. And" to "it, and" in the two instances you have this.

Thanks for revising the comments!
Attachment #679516 - Flags: review?(justin.lebar+bug) → review+
Attached patch patch v3 — — Splinter Review 
Address comment 5.
Attachment #679516 - Attachment is obsolete: true
Attachment #679517 - Flags: review+
Keywords: checkin-needed
If you think we should take this on Aurora (seems like we should), please nom your patch.
Comment on attachment 679517 [details] [diff] [review]
patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 793759 
User impact if declined: Content processes will crash when the ipc connection is broken and they try to call vibrate related functions.
Testing completed (on m-c, etc.): no
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: no
Attachment #679517 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/e74863cebc32
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Attachment #679517 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: