1906132 - adguard extension does not open the popup anymore after last nightly update

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[piradata]

Attachment: Captura de tela 2024-07-03 123954.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:129.0) Gecko/20100101 Firefox/129.0

Steps to reproduce:

open adguard extension on latest nightly build

Actual results:

didnt open

Expected results:

open

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'WebExtensions::Untriaged' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Alex Cornestean]

I reproduced the issue on the latest Nightly (129.0a1/20240704200042) and Beta (128.0/20240704121409) under Windows 10 x64 and Ubuntu 22.04 LTS.
The issue does NOT reproduce on the latest Release (127.0.2/20240624183754).

The popup seems to open when clicking the extension button as there is a thin blank rectangle visible, but the popup content is not rendered.

I also performed a mozregression with the following results:

2024-07-05T09:50:22.012000: DEBUG : Found commit message:
Bug 1874079 - Don't mark a line dirty for clearance if we haven't found any floats. r=dshin

Differential Revision: https://phabricator.services.mozilla.com/D209651

Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7d3c0a98347b6a613de69160201808e405a0a070&tochange=f7306c0a3e5b79b1219be6121cfb8cd186d5275f

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

:emilio, since you are the author of the regressor, bug 1874079, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

[Tracking Requested - why for this release]: Ugh, getting regression reports in rc-week is not fun :(

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1906132 - Explicitly set bresize bit when doing a measuring reflow. r=TYLin,dholbert,#layout

This doesn't change behavior, but it is more explicit.

Make the two resize code-paths invalidate the same way.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1906132 - Propagate BResize flag from viewport frame to root scroll frame. r=TYLin,dholbert,#layout

The regression range is baffling, but it just shows a missing
invalidation.

The extension has worked around this upstream:

https://github.com/AdguardTeam/AdguardBrowserExtension/commit/a48ff54cf6ec7e93f651e8b55b77d08eb0fe1be2

But the root cause is a bug in Firefox. The popup here had styles much
like:

html, body, #root {
  height: 100%;
  min-height: 100%;
  width: auto;
  min-width: 320px;
}

The popup starts off zero-sized, but then gets re-measured async at some
point by the extension code.

Main issue here is that, due to the bresize, we reflow the viewport,
then the html, but the html loses the bresize flag. So we don't reflow
the body element to give it the right height.

Before my patch, the body was reflowed because there was a BFC under it,
so it had the NS_BLOCK_HAS_CLEAR_CHILDREN flag, which ended up papering
over this bug.

I think this can only happen with the special shrink-wrap resize mode,
because it's the only thing that can turn a percentage bsize like 100%
from behaving like a percentage to behave like auto... So I haven't been
able to reproduce outside of our extension popup usage. Otherwise the
percentages resolving to different things would set the bresize flag
appropriately.

Comment 7

[piradata]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

[Tracking Requested - why for this release]: Ugh, getting regression reports in rc-week is not fun :(

sorry kk

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to piradata from comment #7)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

[Tracking Requested - why for this release]: Ugh, getting regression reports in rc-week is not fun :(

sorry kk

emilio's grumbling there was not directed at you. :) Thank you very much for the bug report - I'm glad we caught this before shipping 128 next week! In a perfect world, we would have have already found this bug weeks ago, but better to find it now than to find it after release.

The release process for 128 is already sort of in-motion, so it's a bit of a stretch to get this fix included (in part because it's untested), but hopefully we can...

As emilio noted in the review, it looks like this particular extension has already landed a patch on their side to work around this bug, but apparently that workaround hasn't made it out yet (which is how you managed to hit this bug, I guess; presumably you haven't received the updated extension-version yet). In any case, it's quite possible that other extensions hit this same issue and are similarly-affected.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Indeed, didn't mean to complain about reporting bugs, quite the opposite!

Apparently this has been known since May (https://github.com/AdguardTeam/AdguardBrowserExtension/issues/2817) but it wasn't reported here... :/

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Not going to drive an RC respin over the weekend based on the known impact, but we'll keep an eye out for wider impact after Fx128 ships on Tuesday.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9411385 [details]
Bug 1906132 - Propagate BResize flag from viewport frame to root scroll frame. r=TYLin,dholbert,#layout

Beta/Release Uplift Approval Request

• User impact if declined: Adguard extension popup doesn't show up (at least until the fix for https://github.com/AdguardTeam/AdguardBrowserExtension/issues/2817 is shipped, which is https://github.com/AdguardTeam/AdguardBrowserExtension/commit/a48ff54cf6ec7e93f651e8b55b77d08eb0fe1be2).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Install adguard, try to show the popup.
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very targeted fix. That said, the regressor is also a backout-able one-liner (but it's a performance improvement so slight preference for uplift).
• String changes made/needed: none
• Is Android affected?: No

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d58f607efdb
Explicitly set bresize bit when doing a measuring reflow. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/da96a2244b84
Propagate BResize flag from viewport frame to root scroll frame. r=dholbert

Comment 13

[piradata]

if i update nightly now, this fix would be there already? how can i help testing it?

Comment 14

[Timothy Nikkel (:tnikkel)]

The fix should be in the jul 6 nightly I think.

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/9d58f607efdb
https://hg.mozilla.org/mozilla-central/rev/da96a2244b84

Comment 16

[Alex Cornestean]

Verified as Fixed. Tested on the latest Nightly (129.0a1/20240707205150) under Windows 10, Ubuntu 22.04 LTS and macOS 11.3.1

Clicking the extension button now properly opens and shows the extension pop-up.

Comment 17

[piradata]

also verified just now, its working on the last nightly build 129.0a1 (2024-07-08) (64-bit) :)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Thanks for the confirmation! This fix won't be in the 128.0 release shipping tomorrow, but it is on the radar for a follow-up dot release at some point during this cycle (23-Jul at the latest).

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9411385 [details]
Bug 1906132 - Propagate BResize flag from viewport frame to root scroll frame. r=TYLin,dholbert,#layout

Approved for 128.0.2 and 128.1esr.

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/224e3c1329b8

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/d1f491188d82

Comment 22

[Alex Cornestean]

Verified as Fixed. Tested on the latest Firefox Release (128.0.2/20240719185151 from https://treeherder.mozilla.org/jobs?repo=mozilla-release&revision=4c5c62bac28f37693c0771bebb53ff577ef37ac3) and ESR (128.1.0esr/20240719185227 from https://treeherder.mozilla.org/jobs?repo=mozilla-esr128&revision=269fbb025e79c559a35a41d4ce67f069c2083b94) under Windows 10 and Ubuntu 22.04 LTS.

Clicking the extension button will properly open and show the extension pop-up.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Added to the 128.0.2 relnotes:

Fixed an issue where the Adguard extension popup was not displaying.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9411384 [details]
Bug 1906132 - Explicitly set bresize bit when doing a measuring reflow. r=TYLin,dholbert,#layout

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Turns out to be needed for the complete fix.
• User impact if declined: comment 0
• Fix Landed on Version: 129
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Has been around for a while, only affects (effectively) extension popups.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9411384 [details]
Bug 1906132 - Explicitly set bresize bit when doing a measuring reflow. r=TYLin,dholbert,#layout

Approved for 128.2esr.

Comment 26

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/94b3460d4547