Closed Bug 661410 Opened 10 years ago Closed 10 years ago

Implement UI for out-of-process plugins


(Thunderbird :: General, defect)

Not set


(Not tracked)

Thunderbird 9.0


(Reporter: standard8, Assigned: mconley)




(1 file, 8 obsolete files)

Bug 617906 implemented out-of-process plugins for Thunderbird, but we haven't got around to implementing the text of the UI for it so that if plugins crash, or if there missing plugins, the user gets notified.

We should do that, we should be able to copy most of it from Firefox, probably looking into browser.js.
Attached patch WIP (obsolete) — Splinter Review
In progress, not-working patch.

What I was doing here was copying the Firefox UI code for the plugin handling and migrating functions from their Firefox equivalents to Thunderbird ones.

There's a possibility this could be made into shared code, but I think for now it is easier and quicker to have our own copy.
Assignee: nobody → mconley
I've marked bug 662960 as blocking because the tests I've written use waitFor, as opposed to waitForEval.
Attached patch WIP Patch v1 (obsolete) — Splinter Review
First run at a patch.  Adds a bunch of Mozmill tests.

I haven't connected with Roland yet about the plugin-crash help page, so that's still left to do.
Attachment #551834 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
More protection to ensure that the platform allows oop plugins, and expected minidumps are cleaned up.
Attachment #555198 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
Includes fixes for API rot introduced by bug 686129 and the minidump issue mconley was seeing. (The issue was that the correct call to remove a file is file.remove(false) but what was instead being called was file.remove(). This being in an observer, the exception was eaten silently. I've also included a fix for that.)

I've kicked off try builds:
Attachment #555537 - Attachment is obsolete: true
The try builds failed because of buildbot issues :(

You, sir, are awesome.  Thanks so much for your assistance on this - it was driving me nuts!

Now I just need to determine why the Try build failed out on OSX 10.5.2 (  Or am I correct in remembering that OPP are not enabled/possible for 10.5.2?

Yes. OOPP isn't enabled on 10.5.2.
Note that you might want to change the plugins_run_in_separate_processes function to match something more like this test function:
Attached patch Patch v4 (obsolete) — Splinter Review
Added a more thorough check to ensure that the OS supports OOP plugins (thanks Standard8).

Try builds coming in here:
Attachment #560731 - Attachment is obsolete: true
Attached patch Patch v5 (obsolete) — Splinter Review
Argh, last patch wasn't too smart.  This one is better.

Try builds are here:
Attachment #560921 - Attachment is obsolete: true
Attached patch Patch v6 (obsolete) — Splinter Review
Yay - it looks like my tests finally work! (

Ready for review.
Attachment #560992 - Attachment is obsolete: true
Attachment #561752 - Flags: review?(mbanner)
Comment on attachment 561752 [details] [diff] [review]
Patch v6

Review of attachment 561752 [details] [diff] [review]:

This is mostly looking good, just need to finish reviewing the test* files.

::: mail/app/profile/all-thunderbird.js
@@ +711,5 @@
> +pref("plugins.use_layers", false);
> +pref("plugins.hide_infobar_for_carbon_failure_plugin", false);
> +#endif
> +
> +pref("plugins.update.url", "");

Probably not an ideal url, but it'll do for now.

::: mail/base/content/plugins.js
@@ +40,5 @@
> +                                   ";1",
> +                                   "nsICrashReporter");
> +#endif
> +
> +function getPluginInfo(pluginElement)

I wonder if we should mention in this file that this is largely copied from browser.js.

@@ +84,5 @@
> + *
> + * Currently supported built-ins are LOCALE, APP, and any value from nsIXULAppInfo, uppercased.
> + */
> +function formatURL(aFormat, aIsPref) {
> +  var formatter = Components.classes[";1"]

nit: Can just use Services.urlFormatter (several places)

@@ +110,5 @@
> +  },
> +
> +  get CrashSubmit() {
> +    delete this.CrashSubmit;
> +    Cu.import("resource://gre/modules/CrashSubmit.jsm", this);

When I try this here, I get that Cu is not defined.
Comment on attachment 561752 [details] [diff] [review]
Patch v6

Review of attachment 561752 [details] [diff] [review]:

r=me with the previously mentioned comments fixed.
Attachment #561752 - Flags: review?(mbanner) → review+
Sorry, no level 3 access yet - so checkin-needed.
Checked in:
Closed: 10 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 9.0
You need to log in before you can comment on or make changes to this bug.