Closed Bug 1456171 Opened 6 years ago Closed 6 years ago

Make blocklist service getPluginBlocklistState API asynchronous

Categories

(Toolkit :: Blocklist Implementation, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [fxperf:p1])

Attachments

(1 file, 1 obsolete file)

This is the last major API, and conveniently also the only remaining one that gets called from C++ land, and therefore a bit of a pain.

I have a patch that seems to work...
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [fxperf:p1]
Comment on attachment 8970229 [details]
Bug 1456171 - make getPluginBlocklistState API asynchronous,

https://reviewboard.mozilla.org/r/239032/#review244650

::: dom/plugins/base/nsPluginHost.cpp:2194
(Diff revision 1)
> -      // If the blocklist says it is risky and we have never seen this
> -      // plugin before, then disable it.
> -      if (state == nsIBlocklistService::STATE_SOFTBLOCKED && !seenBefore) {
> -        pluginTag->SetEnabledState(nsIPluginTag::STATE_DISABLED);
> +      nsCOMPtr<nsISupports> result;
> +      blocklist->GetPluginBlocklistState(pluginTag, EmptyString(),
> +                                         EmptyString(), getter_AddRefs(result));
> +      RefPtr<Promise> promise = do_QueryObject(result);
> +      MOZ_ASSERT(promise, "Should always get a promise for plugin blocklist state.");
> +      if (promise) {
> +        // Pass whether we've seen this plugin before. If the plugin is softblocked
> +        // it will be disabled.
> +        RefPtr<BlocklistPromiseHandler> bpHandler = new BlocklistPromiseHandler(pluginTag, !seenBefore);
> +        promise->AppendNativeHandler(bpHandler);

Somewhat scarily, I can't claim to understand the ownership story here.

That is... the JS returns a promise. We get a refptr to that, add the handler, and AFAICT then, synchronously at the end of this method, all of that goes out of scope, so for all I know the promise and its handlers could be destroyed - unless something else hangs on to the promise (I'm assuming the handler doesn't keep a reference to the promise). 

Which it seems to do in my testing (ie it works), but I don't understand what the something is - is it just the fact that there's a handler and the promise implementation? Is it enough that the promise is created in the js context/compartment for the blocklist service and that that's still alive? Do we leak here? Am I just lucky in my testing and could the whole thing be GC'd if the GC happened at an unfortunate time? (And if so, how would I fix that?)
Comment on attachment 8970229 [details]
Bug 1456171 - make getPluginBlocklistState API asynchronous,

https://reviewboard.mozilla.org/r/239032/#review244652


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/plugins/base/nsPluginHost.cpp:313
(Diff revision 1)
> +      MOZ_ASSERT(false, "Shouldn't reject plugin blocklist state request");
> +      MaybeWriteBlocklistChanges();
> +    }
> +
> +  private:
> +    ~BlocklistPromiseHandler() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

 0:08.21     ~BlocklistPromiseHandler() {}
 0:08.21     ^                          ~~
 0:08.21                                = default;
 0:08.21 Warning: modernize-use-auto in /cache/sa-central/dom/plugins/base/nsPluginHost.cpp: use auto when initializing with a cast to avoid duplicating the type name
Comment on attachment 8970229 [details]
Bug 1456171 - make getPluginBlocklistState API asynchronous,

https://reviewboard.mozilla.org/r/239032/#review244784

Thanks

This mostly looks good, but I should probably look at it again after you fix the C++ nits

::: browser/base/content/test/plugins/blocklist_proxy.js:54
(Diff revision 2)
>      await new Promise(r => setTimeout(r, 0));
>      return 0; // STATE_NOT_BLOCKED
>    },
>  
> -  getPluginBlocklistState(aPluginTag, aAppVersion, aToolkitVersion) {
> +  async getPluginBlocklistState(aPluginTag, aAppVersion, aToolkitVersion) {
> +    await new Promise(r => setTimeout(r, 0));

I know eslint and the test harness will complain about "flaky timeouts", but we should probably do something like `setTimeout(r, 150)` here to get us a bit closer to real world behavior.

That probably goes for the add-on state too, I guess.

::: dom/plugins/base/nsPluginHost.cpp:253
(Diff revision 2)
>  static bool UnloadPluginsASAP()
>  {
>    return (Preferences::GetUint(kPrefUnloadPluginTimeoutSecs, kDefaultPluginUnloadingTimeout) == 0);
>  }
>  
> +class BlocklistPromiseHandler : public mozilla::dom::PromiseNativeHandler

`final`

I'm not sure our static analysis code will let you have a non-virtual destructor even with the `final`, but it should be safe as long as `Release` is implemented in the most-derived class.

::: dom/plugins/base/nsPluginHost.cpp:253
(Diff revision 2)
>  static bool UnloadPluginsASAP()
>  {
>    return (Preferences::GetUint(kPrefUnloadPluginTimeoutSecs, kDefaultPluginUnloadingTimeout) == 0);
>  }
>  
> +class BlocklistPromiseHandler : public mozilla::dom::PromiseNativeHandler

This needs to be in a namespace. Having a top-level symbol with a generic name like this really isn't ideal.

::: dom/plugins/base/nsPluginHost.cpp:268
(Diff revision 2)
> +    void
> +    MaybeWriteBlocklistChanges()
> +    {
> +      // A request just resolved. Decrement counter of pending requests and see
> +      // if we need to update the child.
> +      sPendingBlocklistStateRequests--;

This makes me a bit nervous. We should probably either do this in the destructor, or set a flag when we decrement, and decrement in the destructor if we haven't already.

Otherwise, we'll have a non-zero pending request count forever if the promise never settles, or one of the handler functions returns early.

::: dom/plugins/base/nsPluginHost.cpp:270
(Diff revision 2)
> +    {
> +      // A request just resolved. Decrement counter of pending requests and see
> +      // if we need to update the child.
> +      sPendingBlocklistStateRequests--;
> +
> +      RefPtr<nsPluginHost> host = nsPluginHost::GetInst();

Inside the `if`, please.

::: dom/plugins/base/nsPluginHost.cpp:285
(Diff revision 2)
> +        host->SendPluginsToContent();
> +      }
> +    }
> +
> +    void
> +    ResolvedCallback(JSContext *aCx, JS::Handle<JS::Value> aValue) override {

Nit: `{` on its own line.

::: dom/plugins/base/nsPluginHost.cpp:287
(Diff revision 2)
> +    }
> +
> +    void
> +    ResolvedCallback(JSContext *aCx, JS::Handle<JS::Value> aValue) override {
> +      MOZ_ASSERT(aValue.IsInt32(), "Blocklist should always return int32_t");
> +      int32_t newState = aValue.toInt32();

Need to check `aValue.isInt32()`, or we get undefined behavior if JS returns something else.

::: dom/plugins/base/nsPluginHost.cpp:288
(Diff revision 2)
> +
> +    void
> +    ResolvedCallback(JSContext *aCx, JS::Handle<JS::Value> aValue) override {
> +      MOZ_ASSERT(aValue.IsInt32(), "Blocklist should always return int32_t");
> +      int32_t newState = aValue.toInt32();
> +      MOZ_ASSERT(newState > 0 && newState < nsIBlocklistService::STATE_MAX,

`newState >= 0`

::: dom/plugins/base/nsPluginHost.cpp:293
(Diff revision 2)
> +      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;
> +      mTag->GetBlocklistState(&oldState);

May as well make nsIPluginTag.blocklistState infallible, and then to `oldState = mTag->GetBlocklistState()`. Otherwise, you need to check the result, or use `MOZ_ALWAYS_SUCCEEDS`.

::: dom/plugins/base/nsPluginHost.cpp:318
(Diff revision 2)
> +    ~BlocklistPromiseHandler() = default;
> +
> +    RefPtr<nsPluginTag> mTag;
> +    bool mShouldDisableWhenSoftblocked;
> +
> +    NS_DECL_ISUPPORTS

Move this to the top of the class, please.

Also, note that the NS_DECL macros all include `public:`, so having this here makes the rest of the members in the class public.

::: dom/plugins/base/nsPluginHost.cpp:2192
(Diff revision 2)
> -      if (isBlocklistLoaded &&
> -          NS_SUCCEEDED(blocklist->GetPluginBlocklistState(pluginTag, EmptyString(),
> -                                                          EmptyString(), &state))) {
> -        pluginTag->SetBlocklistState(state);
> -      }
>        pluginFile.FreePluginInfo(info);

Urgh. This is scary. My kingdom for a UniquePtr...

::: dom/plugins/base/nsPluginHost.cpp:3507
(Diff revision 2)
> +      nsCOMPtr<nsISupports> result;
>        nsresult rv = blocklist->GetPluginBlocklistState(plugin, EmptyString(),
> -                                                       EmptyString(), &blocklistState);
> +                                                       EmptyString(), getter_AddRefs(result));
>        NS_ENSURE_SUCCESS(rv, rv);
> -      uint32_t oldBlocklistState;
> -      plugin->GetBlocklistState(&oldBlocklistState);
> -      plugin->SetBlocklistState(blocklistState);
> -      blocklistAlteredPlugins |= (oldBlocklistState != blocklistState);
> -      plugin = plugin->mNext;
> +      RefPtr<Promise> promise = do_QueryObject(result);
> +      MOZ_ASSERT(promise, "Should always get a promise for plugin blocklist state.");
> +      if (promise) {
> +        RefPtr<BlocklistPromiseHandler> bpHandler = new BlocklistPromiseHandler(plugin, false);
> +        promise->AppendNativeHandler(bpHandler);
> -    }
> +      }

This is enough duplicated code that it probably makes sense to have a helper for it.

::: dom/plugins/base/nsPluginHost.cpp:3514
(Diff revision 2)
> -                                                       EmptyString(), &blocklistState);
> +                                                       EmptyString(), getter_AddRefs(result));
>        NS_ENSURE_SUCCESS(rv, rv);
> -      uint32_t oldBlocklistState;
> -      plugin->GetBlocklistState(&oldBlocklistState);
> -      plugin->SetBlocklistState(blocklistState);
> -      blocklistAlteredPlugins |= (oldBlocklistState != blocklistState);
> +      RefPtr<Promise> promise = do_QueryObject(result);
> +      MOZ_ASSERT(promise, "Should always get a promise for plugin blocklist state.");
> +      if (promise) {
> +        RefPtr<BlocklistPromiseHandler> bpHandler = new BlocklistPromiseHandler(plugin, false);

Nit: Either pass the `new BlocklistPromiseHandler` directoy to AppendNativeHandler, or do something like:

    auto bpHandler = MakeRefPtr<BlocklistPromiseHandler>(plugin, false);

Same above.

::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_plugin_outdated.js:92
(Diff revision 2)
>    await promiseStartupManager();
>  
>    gBlocklist = Services.blocklist;
>  
>    // should NOT be marked as outdated by the blocklist
> -  Assert.ok(gBlocklist.getPluginBlocklistState(PLUGINS[0], "1", "1.9") == nsIBLS.STATE_NOT_BLOCKED);
> +  Assert.equal(await gBlocklist.getPluginBlocklistState(PLUGINS[0], "1", "1.9"), nsIBLS.STATE_NOT_BLOCKED);

Ugh. Thank you.

::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_severities.js:164
(Diff revision 2)
>    await blocklistUpdated;
>  }
>  
> -function check_plugin_state(plugin) {
> -  return plugin.disabled + "," + plugin.blocklisted;
> +async function check_plugin_state(plugin) {
> +  let blocklistState = await Services.blocklist.getPluginBlocklistState(plugin);
> +  return plugin.disabled + "," + (blocklistState == Services.blocklist.STATE_BLOCKED);

Nit: Can you make this a template string while you're here?

::: toolkit/mozapps/extensions/test/xpcshell/test_pluginBlocklistCtp.js:70
(Diff revision 2)
> +add_task(async function setup() {
> +  createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9");
> +
> +  Services.prefs.setCharPref("extensions.blocklist.url", "http://example.com/data/test_pluginBlocklistCtp.xml");
> +  Services.prefs.setBoolPref("plugin.load_flash_only", false);
> +  startupManager();

Nit: This can be `await promiseStartupManager()` now.
Attachment #8970229 - Flags: review?(kmaglione+bmo)
(In reply to :Gijs (he/him) from comment #2)
> Somewhat scarily, I can't claim to understand the ownership story here.
> 
> That is... the JS returns a promise. We get a refptr to that, add the
> handler, and AFAICT then, synchronously at the end of this method, all of
> that goes out of scope, so for all I know the promise and its handlers could
> be destroyed - unless something else hangs on to the promise (I'm assuming
> the handler doesn't keep a reference to the promise). 

The ownership story is pretty much the same here as it is in JS. The promise keeps a reference to the handler. When it notifies the handler that the promise settles, or the promise is GCed, the handler reference is released.

As long as the handler doesn't keep an explicit reference to the promise, there are no lifetime issues. If it strong references to any objects that could lead to reference cycles with the promise, it needs to be cycle collected. But this one doesn't.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #5)
> ::: dom/plugins/base/nsPluginHost.cpp:293
> (Diff revision 2)
> > +      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;
> > +      mTag->GetBlocklistState(&oldState);
> 
> May as well make nsIPluginTag.blocklistState infallible, and then to
> `oldState = mTag->GetBlocklistState()`. Otherwise, you need to check the
> result, or use `MOZ_ALWAYS_SUCCEEDS`.

This is causing me some pain. To make this infallible, we need to make nsIPluginTag (and nsIFakePluginTag) builtinclass. Doing that breaks places that mock nsIPluginHost::GetPluginTags(), presumably because xpconnect doesn't like you returning things that clearly don't match the interface definition or something. That is, when I rebuild and run toolkit/mozapps/extensions/test/xpcshell/test_blocklist_plugin_outdated.js , which does the above, in nsBlocklistService.js's _blocklistUpdated method we do:

    var phs = Cc["@mozilla.org/plugin/host;1"].
              getService(Ci.nsIPluginHost);
    var plugins = phs.getPluginTags();


where the first succeeds and the second doesn't (or at least logs after it aren't reached), and the test just hangs without any errors. :-(

I assume I should just check the result instead / use MOZ_ALWAYS_SUCCEEDS and file a follow-up for making these classes builtinclass and dealing with the fallout?
Flags: needinfo?(kmaglione+bmo)
Urgh. I searched and couldn't find any JS implementers of that interface, but they just don't define a QI...

We have a native interface for mocking plugin tags:

https://searchfox.org/mozilla-central/source/dom/plugins/base/nsIPluginHost.idl#158

They should really just be using that...

It should be pretty easy to migrate those tests to just use that interface. If you can do it without too much trouble, then please do. Otherwise, yeah, just use MOZ_ALWAYS_SUCCEEDS() and file a follow-up.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #8)
> We have a native interface for mocking plugin tags:
> 
> https://searchfox.org/mozilla-central/source/dom/plugins/base/nsIPluginHost.
> idl#158
> 
> They should really just be using that...
> 
> It should be pretty easy to migrate those tests to just use that interface.
> If you can do it without too much trouble, then please do. Otherwise, yeah,
> just use MOZ_ALWAYS_SUCCEEDS() and file a follow-up.

I poked at this a bit, but the fakeplugintag stuff seems to have been written for mortar/pdfium/pdfjs, and restricts the usable mimetypes, plus there were more tests than I might have liked that relied on the nsIPluginHost + getPluginTags => some JS objects hack. So I'll follow-up, and will try to get to that straight after landing this.
Depends on: 1456324
Comment on attachment 8970229 [details]
Bug 1456171 - make getPluginBlocklistState API asynchronous,

https://reviewboard.mozilla.org/r/239032/#review244852

r=me with the promise handler race fixed.

Bah. Mozreview. I submitted this hours ago, but just found it still in my queue...

::: dom/plugins/base/nsPluginHost.cpp:272
(Diff revision 3)
> +    void
> +    MaybeWriteBlocklistChanges()
> +    {
> +      // If this is the only remaining pending request, check if we need to write
> +      // state and update the child processes.
> +      if (sPendingBlocklistStateRequests == 1 &&

Hm. This is racy. If two resolution handlers run sequentially, for instance, and then two destructors run, we'll never see `Requests == 1` here. We'll also never get to this code if the last pending promise is GCed without resolving.

We should really call this from the destructor, after decrementing, and either only call it from the destructor, or set some sort of sentinal (`mTag = nullptr`?) when we call it from somewhere else.

::: dom/plugins/base/nsPluginHost.cpp:2238
(Diff revision 3)
> +void
> +nsPluginHost::UpdatePluginBlocklistState(nsPluginTag* aPluginTag, bool aShouldSoftblock)
> +{
> +  nsCOMPtr<nsIBlocklistService> blocklist =
> +    do_GetService("@mozilla.org/extensions/blocklist;1");
> +  if (!blocklist) {

Should also MOZ_ASSERT(blocklist)
Attachment #8970229 - Flags: review?(kmaglione+bmo) → review+
Attachment #8970480 - Attachment is obsolete: true
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5d34206436c6
make getPluginBlocklistState API asynchronous, r=kmag
https://hg.mozilla.org/mozilla-central/rev/5d34206436c6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Blocks: 1456677
Component: Blocklist Policy Requests → Blocklist Implementation
You need to log in before you can comment on or make changes to this bug.