Closed Bug 1307516 Opened 5 years ago Closed 5 years ago

TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-policy\test-plugins-policy.js test-plugin-blocked.js test-plugin-crashing.js test-plugin-outdated.js


(Thunderbird :: General, defect)

Not set


(Not tracked)

Thunderbird 52.0


(Reporter: jorgk-bmo, Assigned: jorgk-bmo)




(1 file, 2 obsolete files)

First seen Oct 4, 2016 (in fact on Daily of previous push)

Various plugin related Mozmill tests fail, quoting log:

EXCEPTION: plugin is null
at: test-plugin-blocked.js line 60
at: test-plugin-crashing.js line 78
at: test-plugin-outdated.js line 44

All these lines are preceded by:
let plugin = get_test_plugin();

So obviously this returns null now.
Most likely caused by

Bug 1269807 - Remove support for all NPAPI plugins except for Flash, behind a pref. Tests that use the testplugin for now set the pref to keep it working. This will be disabled for ESR 52, but enabled for release 52. In the next cycle, the pref will be removed and this will be hardcoded. r=jimm

function allow_all_plugins() {
  Services.prefs.setBoolPref("plugin.load_flash_only", false);
Attached patch 1307516.patch (obsolete) — Splinter Review
Let's see whether this works:
Assignee: nobody → jorgk
Attachment #8797683 - Flags: review?(acelists)
Note that there is a follow-up on this, bug 1307501 (meaning, the check for Flash plugin doesn't work either, thus right now *no* plugin is allowed and the tests mail still fail even if plugin.load_flash_only is set to false).
That's not how I read it. With the preference set to true, apparently the Flash plugin does not run when it should; with the preference set to false, all plugins should run.
Oh, ok - you are right. The pref should bypass that check entirely.
Comment on attachment 8797683 [details] [diff] [review]

That didn't work at all :-((
Attachment #8797683 - Flags: review?(acelists)
Comment on attachment 8797683 [details] [diff] [review]

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

Can we just remove all plugin support and remove all the tests? :)

Anyway, the fix seems to NOT work.

::: mail/test/mozmill/content-policy/test-plugins-policy.js
@@ +46,1 @@
>    composeHelper = collector.getModule('compose-helpers');

Please change the var setupmodule to 'function setupModule(module)'.

@@ +46,2 @@
>    composeHelper = collector.getModule('compose-helpers');
>    composeHelper.installInto(module);

This does not need to be a global variable.
(In reply to :aceman from comment #7)
> Can we just remove all plugin support and remove all the tests? :)
So plugin is not equal add-on? I'd hate to remove add-on support ;-(
> Anyway, the fix seems to NOT work.
That's what I said in comment #6.
(In reply to Jorg K (GMT+2) from comment #8)
> So plugin is not equal add-on? I'd hate to remove add-on support ;-(

Figured it out ;-)
So PDF, Flash and Realplayer. Well, only Flash these days.
It is possible that the pref is checked much sooner in the app startup. It may be too late once mozmill passes control to out test code. The plugins are probably enumerated somewhere soon in the m-c startup code.

It seems to me the toolkit tests are all xpcshell.
Yes, it occurred to me that setting the pref comes too late. I have to refresh locally to look into it further. I guess we'd have to set the pref somewhere in the Mozmill framework early on.
Attached patch 1307516-take2.patch (obsolete) — Splinter Review
This will make

Still to do:

content-policy doesn't have a prefs.js, so I need to check how that can be integrated.
Attachment #8797683 - Attachment is obsolete: true
Done. I ran it locally and it passed the test.
Attachment #8797952 - Attachment is obsolete: true
Attachment #8797955 - Flags: review?(acelists)
Comment on attachment 8797955 [details] [diff] [review]
1307516-take2.patch (v2).

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

Nice, works for me.
Attachment #8797955 - Flags: review?(acelists) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
Duplicate of this bug: 1308722
You need to log in before you can comment on or make changes to this bug.