1607366 - Show an infobar warning if a user runs Fission without WebRender enabled

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, task, P1)

Description

[Chris Peterson [:cpeterson]]

That would reduce bug reports about non-WebRender Fission issues (like bug 1606316), but still allow people to test Fission without WebRender.

We should show the infobar warning on every launch. That will annoy users, but we don't want them to forget they are testing an unsupported configuration.

Are any tests running Fission without WebRender?

Comment 1

[David Teller [:Yoric] - still alive but not very active]

So, we can detect whether the window is running WebRender using nsIGfxInfo and nsIDocShell's or tabbrowser's useRemoteSubframes.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

I'll try and implement this as an add-on, if I can find how to access xpcom from a system add-on.

Comment 3

[:Gijs (he/him)]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #2)

I'll try and implement this as an add-on, if I can find how to access xpcom from a system add-on.

I think other people have said this, but please do not use system add-ons to implement this infobar - we're trying to get rid of them as much as possible as they have performance as well as other issues, and aren't normally necessary unless we're updating them regularly, away from the usual release cycle, which I don't think will be the case here.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

@Gijs: Noted.

@cpeterson: Is this something we'll want localized? Also, should this be Nightly-only?

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attaching a first prototype. It eyeball-works but I haven't managed to CI test it. For some reason, aBrowser.getNotificationWithValue doesn't ever return a notification, maybe I'm running the code from the wrong process.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1607366 - Display a warning when we're running Fission without WebRender;r?gijs

Comment 7

[Chris Peterson [:cpeterson]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #4)

@cpeterson: Is this something we'll want localized? Also, should this be Nightly-only?

This info bar can be Nightly only. Fission cannot be enabled in Beta or Release channels, at this time.

Localizing the message is not a high priority. This will be a temporary, Nightly-only warning for people who have manually enabled the fission.autostart pref. Those people likely learned about the pref from reading English comments in Bugzilla or email.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f52616a34d3
Display a warning when we're running Fission without WebRender;r=Gijs

Comment 9

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 3f52616a34d3 (Bug 1607366) for causing bc failures in browser_startup_flicker.js CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=8f51a5fac21cb52e2ddb647f0b99a9bfccb41f6a&selectedJob=290364879

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=290364879&repo=autoland&lineNumber=3830

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Florian, is there a way to somehow whitelist this warning for browser_startup_flicker.js and/or to remove browser_startup_flicker.js when we're running with Fission and without WebRender (which is an unsupported combo)?

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f03b006038b5
Backed out changeset 3f52616a34d3 for causing bc failures in browser_startup_flicker.js CLOSED TREE

Comment 12

[Florian Quèze [:florian]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #10)

Florian, is there a way to somehow whitelist this warning for browser_startup_flicker.js and/or to remove browser_startup_flicker.js when we're running with Fission and without WebRender (which is an unsupported combo)?

If this is an unsupported combo, should we disable the whole test job?

But otherwise, yes, you can add an early return at the beginning of the browser_startup_flicker.js test if fission is enabled and webrender is not.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

(In reply to Florian Quèze [:florian] from comment #12)

If this is an unsupported combo, should we disable the whole test job?

I don't really know why we run it. Maybe ask Nika?

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1607366 - Deactivate browser_startup_flicker when we have Fission without WebRender;r?florian

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7bdea74790de
Deactivate browser_startup_flicker when we have Fission without WebRender;r=florian

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/7bdea74790de

Comment 17

[:Gijs (he/him)]

David, you marked the "real" patch of adding the notification bar obsolete, but AFAICT that patch is not in current central/autoland - shouldn't it be relanded now that the startup_flicker thing has landed?

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Oops, thanks!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22f74b61de35
Display a warning when we're running Fission without WebRender;r=Gijs

Comment 20

[Alexandru Michis [:malexandru]]

Backed out changeset 22f74b61de35 for causing mochitest fission failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/8bf12471ce141480bbe25846aaecd914bd8054f8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=mochitest%2Cfission&tochange=5490c9f430b8ac94093156d0490b95f31507e3cf&fromchange=48672c0423781b36e2524db84791cb259e21f47c&selectedJob=290780638

Failure logs:

• On browser_urlbar_keyed_search.js : https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=290780638&repo=autoland&lineNumber=4707
• On browser_UnsubmittedCrashHandler.js : https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=290781754&repo=autoland&lineNumber=20840

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Looks like I need to remove a few more tests when we're running with fission and without webrender.

Comment 22

[:Gijs (he/him)]

If this notification bar is only there for users, perhaps an easier fix would be to just avoid showing the notification in automation? You can test for this using Cu.isInAutomation.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

(In reply to :Gijs (he/him) from comment #22)

If this notification bar is only there for users, perhaps an easier fix would be to just avoid showing the notification in automation? You can test for this using Cu.isInAutomation.

I guess I could get rid of my tests and do that. Since you're my reviewer, what's the best option in your eye?

Comment 24

[:Gijs (he/him)]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #23)

(In reply to :Gijs (he/him) from comment #22)

If this notification bar is only there for users, perhaps an easier fix would be to just avoid showing the notification in automation? You can test for this using Cu.isInAutomation.

I guess I could get rid of my tests and do that. Since you're my reviewer, what's the best option in your eye?

Factor out the actual creation of the notification box into a function, only call it when Cu.isInAutomation is false, and then in your test, manually call that function? That way we can have our cake and eat it.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

I fear that that starts sounding a bit complicated to test what is essentially a one-liner, no?

Where would you like this function/method published? As a member of gBrowser?

Comment 26

[:Gijs (he/him)]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #25)

I fear that that starts sounding a bit complicated to test what is essentially a one-liner, no?

I'd also be OK with removing the test, if you prefer.

Where would you like this function/method published? As a member of gBrowser?

It'd be on FissionTestingUI.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10a8c9467b67
Display a warning when we're running Fission without WebRender;r=Gijs

Comment 28

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/10a8c9467b67

Comment 29

[Laurențiu Nicola]

This is a bit annoying on configurations where WebRender is enabled by default.

Comment 30

[David Teller [:Yoric] - still alive but not very active]

(In reply to Laurențiu Nicola from comment #29)

This is a bit annoying on configurations where WebRender is enabled by default.

Wait, we have configurations where WebRender is enabled and this shows up by error? If so, could you file a bug and Cc me?

Comment 31

[Laurențiu Nicola]

i can't file a new bug right now, but I've seen it on Linux. I think the WR pref is also disabled on qualified Windows systems.