Closed Bug 688083 Opened 13 years ago Closed 13 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 10

People

(Reporter: felix, Assigned: felix)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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-
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+
https://hg.mozilla.org/mozilla-central/rev/fc2cf14697f8
Status: NEW → RESOLVED
Closed: 13 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
Regressions: 740304
You need to log in before you can comment on or make changes to this bug.