1456324 - Stop mocking nsIPluginHost manually in tests, use nsIFakePluginTag instead, make nsI*PluginTag builtinclass

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

[:Gijs (he/him)]

Making some attributes infallible in bug 1456171 failed because we need to make the classes `builtinclass`, which in turn fails because we have tests that mock nsIPluginHost and then fake the plugintags with vanilla JS objects, which upsets xpconnect if you make nsIPluginTag builtinclass.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1456324: Part 1 - Update tests to use nsIFakePluginTag rather than JS mocks.

This will allow us to make nsIPluginTag a builtin class.

This patch also factors out some common logic from AOM plugin tests.

Review commit: https://reviewboard.mozilla.org/r/239504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239504/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1456324: Mark infallible nsIPluginTag getters as infallible.

Review commit: https://reviewboard.mozilla.org/r/239506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239506/

Comment 3

[:Gijs (he/him)]

Comment on attachment 8970755 [details]
Bug 1456324: Part 1 - Update tests to use nsIFakePluginTag rather than JS mocks.

https://reviewboard.mozilla.org/r/239504/#review245302

::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_appversion.js:285
(Diff revision 1)
> +  ).catch(() => { /* ignore exceptions; the following test will fail anyway. */ });
> +
> +  for (let [i, addon] of ADDONS.entries()) {
>      var blocked = addons[i].blocklistState == Ci.nsIBlocklistService.STATE_BLOCKED;
> -    equal(blocked, ADDONS[i][test],
> +    equal(blocked, addon[test],
>            `Blocklist state should match expected for extension ${addons[i].id}, test ${test}`);

Nit: addon.id

::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_plugin_outdated.js:19
(Diff revision 1)
>  gTestserver.registerDirectory("/data/", do_get_file("data"));
>  
> +class MockPlugin extends MockPluginTag {
> +  constructor(opts) {
> +    super(opts, opts.enabledState);
> +    this.blocklisted = opts.blocklisted;

Is there actually a point to this blocklisted = false property in this test (and the next one in this cset)? I don't see one off-hand.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8970756 [details]
Bug 1456324: Mark infallible nsIPluginTag getters as infallible.

https://reviewboard.mozilla.org/r/239506/#review245306

::: dom/plugins/base/nsPluginHost.cpp:308
(Diff revision 1)
>        int32_t newState = aValue.toInt32();
>        MOZ_ASSERT(newState >= 0 && newState < nsIBlocklistService::STATE_MAX,
>          "Shouldn't get an out of bounds blocklist state");
>  
>        // Check the old and new state and see if there was a change:
> -      uint32_t oldState = nsIBlocklistService::STATE_NOT_BLOCKED;
> +      uint32_t oldState = mTag->GetBlocklistState();

Is it worth filing a good first bug to update other callsites for these properties to use the infallible versions of their getters?

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8970755 [details]
Bug 1456324: Part 1 - Update tests to use nsIFakePluginTag rather than JS mocks.

https://reviewboard.mozilla.org/r/239504/#review245302

> Is there actually a point to this blocklisted = false property in this test (and the next one in this cset)? I don't see one off-hand.

Apparently not. *shrug*

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8970756 [details]
Bug 1456324: Mark infallible nsIPluginTag getters as infallible.

https://reviewboard.mozilla.org/r/239506/#review245306

> Is it worth filing a good first bug to update other callsites for these properties to use the infallible versions of their getters?

Yeah, I guess it couldn't hurt.

Comment 7

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a593e7961a2d230a729bd9060d2c83ea7ae86e9
Bug 1456324: Part 1 - Update tests to use nsIFakePluginTag rather than JS mocks. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbcb2bea294230c9dcd3edc67b5728a9ae1d9f75
Bug 1456324: Part 2 - Mark infallible nsIPluginTag getters as infallible. r=Gijs

Comment 8

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b66db25a4e39f94a90c8161187466b61a7a2d03
Bug 1456324: Follow-up: Fix assertion in debug builds. r=bustage

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3a593e7961a2
https://hg.mozilla.org/mozilla-central/rev/cbcb2bea2942
https://hg.mozilla.org/mozilla-central/rev/9b66db25a4e3