Fix dom.ipc.plugins.asyncInit on e10s-enabled channels

RESOLVED WONTFIX

Status

()

Core
Plug-ins
P5
normal
RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(e10s+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
This pref should be enabled by default when e10s is deactivated (at least until the e10s-specific asyncInit bugs are ironed out). Right now it is disabled by default even when e10s is disabled.
(Assignee)

Comment 1

2 years ago
Created attachment 8641816 [details] [diff] [review]
Patch
Attachment #8641816 - Flags: review?(jmathies)
(Assignee)

Comment 2

2 years ago
Created attachment 8641856 [details] [diff] [review]
Patch

Whoops, missed an #include
Attachment #8641816 - Attachment is obsolete: true
Attachment #8641816 - Flags: review?(jmathies)
Attachment #8641856 - Flags: review?(jmathies)
(Assignee)

Comment 3

2 years ago
Created attachment 8641861 [details] [diff] [review]
Telemetry Patch

To make sure that telemetry reflects reality. I'll revert this when the e10s bugs in asyncInit are resolved.
Attachment #8641861 - Flags: review?(gfritzsche)

Comment 4

2 years ago
Comment on attachment 8641856 [details] [diff] [review]
Patch

Having that pref not take effect is a bit misleading but it's an internal thing I guess, seems ok.
Attachment #8641856 - Flags: review?(jmathies) → review+
Comment on attachment 8641861 [details] [diff] [review]
Telemetry Patch

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1101,5 @@
>      let ret = {
>        reason: reason,
>        revision: HISTOGRAMS_FILE_VERSION,
> +      asyncPluginInit: (Preferences.get(PREF_ASYNC_PLUGIN_INIT, false) &&
> +                        !Services.appinfo.browserTabsRemoteAutostart),

I think we should just fix bug 1179189 instead, making settings.e10sEnabled use |appinfo.browserTabsRemoteAutostart| - then we don't need to change anything about asyncPluginInit.
Attachment #8641861 - Flags: review?(gfritzsche)
Currently you can already filter this out in analysis using the E10S_AUTOSTART histogram.
tracking-e10s: --- → +
(Assignee)

Comment 7

2 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> Comment on attachment 8641861 [details] [diff] [review]
> Telemetry Patch
> 
> Review of attachment 8641861 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ +1101,5 @@
> >      let ret = {
> >        reason: reason,
> >        revision: HISTOGRAMS_FILE_VERSION,
> > +      asyncPluginInit: (Preferences.get(PREF_ASYNC_PLUGIN_INIT, false) &&
> > +                        !Services.appinfo.browserTabsRemoteAutostart),
> 
> I think we should just fix bug 1179189 instead, making settings.e10sEnabled
> use |appinfo.browserTabsRemoteAutostart| - then we don't need to change
> anything about asyncPluginInit.

It's not clear to me how that bug applies to this one: the other patch in this bug modifies asyncInit to be disabled when e10s is enabled (temporarily, until some remaining issues are ironed out), even when the pref is set to true. The point of this patch is to ensure that the state of the flag in telemetry is synchronized with the effective state of the feature in the plugin code.
Flags: needinfo?(gfritzsche)
I am saying that you can already pull this data by looking at both asyncPluginInit & E10S_AUTOSTART (or now e10sEnabled), so i'm not sure why we need to duplicate this logic temporarily.

Alternatively we could add, say, nsIPluginHost.asyncPluginInit and use that in both places.
Flags: needinfo?(gfritzsche)

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f3a6818c4d
(Assignee)

Comment 10

2 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> I am saying that you can already pull this data by looking at both
> asyncPluginInit & E10S_AUTOSTART (or now e10sEnabled), so i'm not sure why
> we need to duplicate this logic temporarily.
> 
> Alternatively we could add, say, nsIPluginHost.asyncPluginInit and use that
> in both places.

I don't really feel like adding a bunch of nsPluginHost goop to accommodate an ephemeral change. I guess we'll stick with pulling e10sEnabled in analyses for now.
backed out for mochitest-3 failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/26cc89d943d6

https://treeherder.mozilla.org/logviewer.html#?job_id=12534509&repo=mozilla-inbound
Flags: needinfo?(aklotz)
(Assignee)

Updated

2 years ago
Depends on: 1172627
(Assignee)

Comment 12

2 years ago
I've added a dependency on bug 1172627; look like I need to fix that first.
Flags: needinfo?(aklotz)
Priority: -- → P5
(Assignee)

Comment 13

2 years ago
async init is on hold. Setting this to WONTFIX. If we resurrect it, we will need to bring this back.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.