With in-content plugin crash, send crash report option should be synced amongst windows/tabs

RESOLVED FIXED in Firefox 10

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: felix, Assigned: felix)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
When a plugin crashes (like flash) we sometimes show an in-page error box indicating that they can send crash report and/or reload the page. bug 665196 is going to make sending a crash report a checkbox option. This checkbox should be synced amongst all in-page error boxes (of which there could be multiple across windows and tabs) whenever the plugin crashes.
(Assignee)

Comment 1

6 years ago
Created attachment 562227 [details] [diff] [review]
Plugin Crash: Sync Submit Report Checkbox With Pref Between Instances

Summary of Changes:
- Added observer for each instance via closureHack to observe changes to gCrashReporter.submitReports
- Each plugin instance will send notification when send crash report checkbox is clicked
Attachment #562227 - Flags: review?(dolske)
Comment on attachment 562227 [details] [diff] [review]
Plugin Crash: Sync Submit Report Checkbox With Pref Between Instances

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

Need to move the notifyObservers, but otherwise looks good.

::: browser/base/content/browser.js
@@ +7079,5 @@
> +      // notifies plugin crash instances to sync submit reports checkbox
> +      submitChk.addEventListener("click", function() {
> +        gCrashReporter.submitReports = this.checked;
> +        Services.obs.notifyObservers(
> +          null, "submit-reports-pref-changed", pluginDumpID);

The notification should really be sent instead by the code that implements the .submitReports property...

That would be SetSubmitReports() in toolkit/crashreporter/nsExceptionHandler.cpp. Look around the tree for "obsService->NotifyObservers" for how to notify from native code, it's basically the same as in JS.

No need to pass along the pluginDumpID, it's fine to sync the pref globally as it's not really specific to a particular crash.

@@ +7105,2 @@
>      if (doPrompt) {
> +      let submitReportsPrefObserver = {

You could fold this code into the existing observer (by checking |subject| to see how it's being called), but it's fine either way.

@@ +7109,5 @@
> +        observe : function(subject, topic, data) {
> +          let submitChk = doc.getAnonymousElementByAttribute(
> +                            plugin, "class", "pleaseSubmitCheckbox");
> +          // Ignore notifications for other crashes.
> +          if (data == pluginDumpID) {

Don't need this check.
Attachment #562227 - Flags: review?(dolske) → review-
(Assignee)

Comment 3

6 years ago
Created attachment 564308 [details] [diff] [review]
Plugin Crash: Sync Submit Report Checkbox With Pref Between Instances

Additional Changes:
- Made request changes above
- Didn't fold the observers. I just thought "crash-status" isn't very indicative of a notification name.
Attachment #562227 - Attachment is obsolete: true
Attachment #564308 - Flags: review?(dolske)
Attachment #564308 - Flags: review?(dolske) → review+

Comment 4

6 years ago
https://hg.mozilla.org/integration/fx-team/rev/fc2cf14697f8
Whiteboard: [fixed-in-fx-team]

Comment 5

6 years ago
https://hg.mozilla.org/mozilla-central/rev/fc2cf14697f8
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 10
Comment on attachment 564308 [details] [diff] [review]
Plugin Crash: Sync Submit Report Checkbox With Pref Between Instances

> nsresult SetSubmitReports(bool aSubmitReports)
> {
>-    return PrefSubmitReports(&aSubmitReports, true);
>+    nsresult rv;
>+
>+    nsCOMPtr<nsIObserverService> obsServ =
>+      mozilla::services::GetObserverService();
>+    if (!obsServ) {
>+      return NS_ERROR_FAILURE;
>+    }
>+
>+    rv = PrefSubmitReports(&aSubmitReports, true);

Mozilla style is to declare rv here, when it's first used.
Blocks: 697964
You need to log in before you can comment on or make changes to this bug.