Closed Bug 1715788 Opened 3 years ago Closed 2 years ago

Ensure we can practically use the gfx blocklist for Android

Categories

(Core :: Graphics, task)

Unspecified
Android
task

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: robwu, Unassigned)

References

Details

In bug 1714673 I discovered that the gfx blocklist for Android hasn't worked for the past five years (https://bugzilla.mozilla.org/show_bug.cgi?id=1714673#c2). I've fixed the issue, but wonder whether we need this functionality at all for Android. Since the broken feature has gone unnoticed for so long, it may be safe to assume that the remote gfx blocklist is not necessary. In that case, we can remove the functionality from Android, which reduces the maintenance burden of the blocklist implementation (both code and services).

The remote blocklist implementation (Blocklist.jsm) is only used for the gfx blocklist and the add-ons blocklist. The "blocklists" bucket supporting the latter will become obsolete in bug 1639050. If we remove support for the remote gfx blocklist on Android, then we don't need to fetch the "blocklists" collection at all on Android.

If the gfx blocklist is ever needed on Android, then we can use the compile-time gfx blocklist that's already present, and publish a new GeckoView+Fenix release.

Jeff, what's your opinion on this? Should we disable the remote gfx blocklist on Android?

And if not, is there current documentation on how it should be used? I don't see anything at https://wiki.mozilla.org/Blocklisting/Graphics

Flags: needinfo?(jmuizelaar)

I'm inclined to try and keep it. However, I don't have a feeling for costly it is to do new fenix release. I'll ask around about that.

After asking around, it sounds like its not that pleasant to do Fenix dot releases so it's probably worth keeping

Flags: needinfo?(jmuizelaar)
Flags: needinfo?(rob)

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)

After asking around, it sounds like its not that pleasant to do Fenix dot releases so it's probably worth keeping

Rob, I'm guessing this means we should wontfix this? Is there anything else we could/should do here?

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

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)

After asking around, it sounds like its not that pleasant to do Fenix dot releases so it's probably worth keeping

Rob, I'm guessing this means we should wontfix this? Is there anything else we could/should do here?

Review whether the blocklist documentation at https://wiki.mozilla.org/Blocklisting/Graphics is still accurate, update if needed and then close this bug.

Flags: needinfo?(rob)

(In reply to Rob Wu [:robwu] from comment #5)

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

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)

After asking around, it sounds like its not that pleasant to do Fenix dot releases so it's probably worth keeping

Rob, I'm guessing this means we should wontfix this? Is there anything else we could/should do here?

Review whether the blocklist documentation at https://wiki.mozilla.org/Blocklisting/Graphics is still accurate, update if needed and then close this bug.

I added a note about bug 1714673 to the wiki. Jeff, can you review the wiki and check/update it as necessary? Thanks.

Flags: needinfo?(jmuizelaar)

The wiki says:

blocklist entries fetched after installation did not apply on Android; only the list that shipped with the application was used.

This is ambiguous, so just to clarify: On Android, until 91, the only blocklist entries that work are the ones shipped at compile time.

The gfx blocklist (from Blocklist.jsm) isn't even populated on Android because the gfx.json file is not packaged with Android (it is not in mobile/android/installer/package-manifest.in, and neither in services/settings/dumps/blocklists/moz.build).

(In reply to Rob Wu [:robwu] from comment #7)

The wiki says:

blocklist entries fetched after installation did not apply on Android; only the list that shipped with the application was used.

This is ambiguous, so just to clarify: On Android, until 91, the only blocklist entries that work are the ones shipped at compile time.

Updated this with similar wording to this comment.

The gfx blocklist (from Blocklist.jsm) isn't even populated on Android because the gfx.json file is not packaged with Android (it is not in mobile/android/installer/package-manifest.in, and neither in services/settings/dumps/blocklists/moz.build).

Is this something we should fix separately, then? I know there are size concerns with android, but the gfx blocklist isn't nearly as big as the addon blocklist was, and so shipping the dump seems useful?

Flags: needinfo?(rob)

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

(In reply to Rob Wu [:robwu] from comment #7)

The gfx blocklist (from Blocklist.jsm) isn't even populated on Android because the gfx.json file is not packaged with Android (it is not in mobile/android/installer/package-manifest.in, and neither in services/settings/dumps/blocklists/moz.build).

Is this something we should fix separately, then? I know there are size concerns with android, but the gfx blocklist isn't nearly as big as the addon blocklist was, and so shipping the dump seems useful?

None of the entries in services/settings/dumps/blocklists/gfx.json apply to Android (search for "Android" and you'll find zero results; "os": "All" doesn't match Android).

Because of this lack of entries, and the lack of need for this feature in the past FIVE years, I would recommend to just disable this remote fetching. If there is ever a gfx regression on mobile, then a dot release could be created. It's not pleasant, but it has not happened either.

Flags: needinfo?(rob)

jrmuizel is out until July 19th - hey jimm, perhaps someone else from the Graphics team could weigh in on this?

Flags: needinfo?(jmathies)

Jamie, do you have any leaning as to what we want to do here?

Flags: needinfo?(jmathies) → needinfo?(jnicol)

i.e. do you think it's ok to drop support for the blocklist and just use dot releases?

Flags: needinfo?(jmuizelaar)

I can't comment on the burden of maintaining the code, nor on how much work dot releases are for the release team. But I can say that if this feature is now working there will be many opportunities to use it. I wouldn't say its not working has gone unnoticed for 5 years - we knew it not to work, but perhaps overestimated how difficult it would be to get to work.

We've had to do a lot of dot releases over the course of the webrender rollout due to driver bugs causing severe glitches, crashes, or making the browser completely unusable. Due to very low nightly and beta populations we often don't have such issues reported until they hit release.

While webrender rollout is basically complete now, these bugs can still occur due to driver updates on the phone (as was in the case in bug 1688017), landing improvements which require new GL features (eg bug 1683936, causing a dot release on Dec 23rd), or even simple refactorings that should have no effect (eg bug 1709548).

Assuming frequent dot releases are a greater pain to do than maintaining the code, I'd argue in favour of keeping the feature.

Flags: needinfo?(jnicol)

Given comment 13, is this a WONTFIX?

Flags: needinfo?(rob)
See Also: → 1722341

I can't comment on the burden of maintaining the code, nor on how much work dot releases are for the release team. But I can say that if this feature is now working there will be many opportunities to use it. I wouldn't say its not working has gone unnoticed for 5 years - we knew it not to work, but perhaps overestimated how difficult it would be to get to work.

Looks like a working blocklist can potentially be useful in avoiding dot releases. If we want to support this feature for use in the future, I suggest the following action items:

  1. (filed new bug: bug 1722341) Bundle gfx.json with Android. Alternatively, since it's 32 kb at the moment, none of which relevant to Android, we could also decide to not do this, and rely on the remote updating functionality of the blocklist. If 32kb is acceptable (in compressed form it's likely even smaller), then I suggest to bundle gfx.json for simplicity (and enable the functionality from bug 1717068 for the gfx blocklist).
  2. Verify that the implemented blocklist supports the use case, by converting the bugs from comment 13 to examples of blocklist entries. This would be a good practice and a future reference in case we run in a new situation where the functionality is desired.

I can take care of 1 if needed.
The second task would better be suited for someone who is more familiar with gfx.

Flags: needinfo?(rob)

(In reply to Rob Wu [:robwu] from comment #15)

I can't comment on the burden of maintaining the code, nor on how much work dot releases are for the release team. But I can say that if this feature is now working there will be many opportunities to use it. I wouldn't say its not working has gone unnoticed for 5 years - we knew it not to work, but perhaps overestimated how difficult it would be to get to work.

Looks like a working blocklist can potentially be useful in avoiding dot releases. If we want to support this feature for use in the future, I suggest the following action items:

  1. (filed new bug: bug 1722341) Bundle gfx.json with Android. Alternatively, since it's 32 kb at the moment, none of which relevant to Android, we could also decide to not do this, and rely on the remote updating functionality of the blocklist. If 32kb is acceptable (in compressed form it's likely even smaller), then I suggest to bundle gfx.json for simplicity (and enable the functionality from bug 1717068 for the gfx blocklist).
  2. Verify that the implemented blocklist supports the use case, by converting the bugs from comment 13 to examples of blocklist entries. This would be a good practice and a future reference in case we run in a new situation where the functionality is desired.

I can take care of 1 if needed.
The second task would better be suited for someone who is more familiar with gfx.

Jamie, can someone from your team commit to picking up (2) here?

Flags: needinfo?(jnicol)

Gonna just morph this bug so the conversation matches the summary better. :-)

Summary: Remove remote gfx blocklist for Android → Ensure we can practically use the gfx blocklist for Android

Sure, I can do that.

We don't actually block webrender any more for those examples as we found workarounds for the bugs. In fact we don't block webrender at all currently. However, we do block individual features of webrender, so I will attempt to convert those to use the downloadable blocklist.

Flags: needinfo?(jnicol)

(In reply to Jamie Nicol [:jnicol] from comment #18)

Sure, I can do that.

We don't actually block webrender any more for those examples as we found workarounds for the bugs. In fact we don't block webrender at all currently. However, we do block individual features of webrender, so I will attempt to convert those to use the downloadable blocklist.

OK. Going to move this bug over to graphics in order to for this work to be triaged & happen.

Component: Blocklist Implementation → Graphics
Product: Toolkit → Core
Severity: -- → S3
Depends on: 1749520
Depends on: 1761953
Depends on: 1722341
See Also: 1722341

Jamie, Agi fixed bug 1722341 to bundle gfx.json blocklist in our Android builds. How do we verify that this bug to use the gfx blocklist is working on Android?

No longer depends on: 1722341
Flags: needinfo?(jnicol)
See Also: → 1722341

In a local geckoview example build, I modified this entry. So that vendor = "Qualcomm", devices = "Adreno (TM) 540" and maxVersion = 105.

On a fresh install on my pixel 2, on the first run the blocklist gets imported, but webrender is still enabled (as expected). On the second run webrender is blocked and we fall back to software webrender.

So it seems to work as expected.

Flags: needinfo?(jnicol)

Thanks for testing. I'll resolve this bug as WORKSFORME.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.