Last Comment Bug 688083 - With in-content plugin crash, send crash report option should be synced amongst windows/tabs
: With in-content plugin crash, send crash report option should be synced among...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Felix Fung (:felix)
:
Mentors:
Depends on: 665196
Blocks: 697964
  Show dependency treegraph
 
Reported: 2011-09-20 19:26 PDT by Felix Fung (:felix)
Modified: 2011-10-28 05:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Plugin Crash: Sync Submit Report Checkbox With Pref Between Instances (5.98 KB, patch)
2011-09-24 00:33 PDT, Felix Fung (:felix)
dolske: review-
Details | Diff | Review
Plugin Crash: Sync Submit Report Checkbox With Pref Between Instances (7.35 KB, patch)
2011-10-03 13:28 PDT, Felix Fung (:felix)
dolske: review+
Details | Diff | Review

Description Felix Fung (:felix) 2011-09-20 19:26:40 PDT
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.
Comment 1 Felix Fung (:felix) 2011-09-24 00:33:35 PDT
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
Comment 2 Justin Dolske [:Dolske] 2011-10-02 16:24:30 PDT
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.
Comment 3 Felix Fung (:felix) 2011-10-03 13:28:16 PDT
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.
Comment 5 :Margaret Leibovic 2011-10-27 11:33:59 PDT
https://hg.mozilla.org/mozilla-central/rev/fc2cf14697f8
Comment 6 :Ms2ger 2011-10-27 12:00:02 PDT
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.

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