661410 - Implement UI for out-of-process plugins

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mark Banner (:standard8)]

Attachment: WIP

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.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I've marked bug 662960 as blocking because the tests I've written use waitFor, as opposed to waitForEval.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch v1

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.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Try builds coming in here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=d9d6f9b28219

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

More protection to ensure that the platform allows oop plugins, and expected minidumps are cleaned up.

Comment 6

[Rain [:rain1] (inactive)]

Attachment: patch v3

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: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=0466293b5589

Comment 7

[Rain [:rain1] (inactive)]

The try builds failed because of buildbot issues :(

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Sid:

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 (http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1316302549.1316303253.12791.gz).  Or am I correct in remembering that OPP are not enabled/possible for 10.5.2?

-Mike

Comment 9

[Mark Banner (:standard8)]

Yes. OOPP isn't enabled on 10.5.2.

Comment 10

[Mark Banner (:standard8)]

Note that you might want to change the plugins_run_in_separate_processes function to match something more like this test function:

http://hg.mozilla.org/mozilla-central/annotate/5319b0100025/layout/tools/reftest/reftest.js#l490

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4

Added a more thorough check to ensure that the OS supports OOP plugins (thanks Standard8).

Try builds coming in here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=bef328eb7431

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v5

Argh, last patch wasn't too smart.  This one is better.

Try builds are here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=4def3033b689

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v6

Yay - it looks like my tests finally work! (http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=40a1c58b4722)

Ready for review.

Comment 14

[Mark Banner (:standard8)]

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", "https://www.mozilla.com/%LOCALE%/plugincheck/");

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

::: mail/base/content/plugins.js
@@ +40,5 @@
> +                                   "@mozilla.org/xre/app-info;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["@mozilla.org/toolkit/URLFormatterService;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 15

[Mark Banner (:standard8)]

Comment on attachment 561752 [details] [diff] [review]
Patch v6

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

r=me with the previously mentioned comments fixed.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v7 (r+'d by Standard8)

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Sorry, no level 3 access yet - so checkin-needed.

Comment 18

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/9fa12b677b96