Should not call hal_sandbox::Vibrate when ipc is broken

RESOLVED FIXED in Firefox 18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: slee, Assigned: slee)

Tracking

unspecified
mozilla19
x86_64
All
Points:
---

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

6 years ago
Summary: Should not call hal_sandbox::Vibrate when child process is dead → Should not call hal_sandbox::Vibrate when ipc is broken
(Assignee)

Comment 1

6 years ago
Created attachment 679070 [details] [diff] [review]
patch

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.
(Assignee)

Comment 4

6 years ago
Created attachment 679516 [details] [diff] [review]
patch v2

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+
(Assignee)

Comment 6

6 years ago
Created attachment 679517 [details] [diff] [review]
patch v3

Address comment 5.
Attachment #679516 - Attachment is obsolete: true
Attachment #679517 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
If you think we should take this on Aurora (seems like we should), please nom your patch.
(Assignee)

Comment 8

6 years ago
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?

Comment 10

6 years ago
https://hg.mozilla.org/mozilla-central/rev/e74863cebc32
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

6 years ago
Attachment #679517 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/424e76e7dc62
status-firefox18: --- → fixed
status-firefox19: --- → fixed
You need to log in before you can comment on or make changes to this bug.