Closed Bug 1715369 Opened 3 years ago Closed 3 years ago

hCaptcha in ProtonMail password reset page doesn't work with Fission enabled

Categories

(Core :: Panning and Zooming, defect, P1)

Product:
Core
Component:
Panning and Zooming
Version:
Firefox 89
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
91 Branch
Project Flags:
Fission Milestone M7a
Tracking Flags:
Tracking Status
firefox-esr78 --- disabled
firefox89 --- disabled
firefox90 --- disabled
firefox91 --- fixed

People

(Reporter: mystiquewolf, Assigned: tnikkel)

References

Details

Attachments

(3 files)

Troubleshooting info
34.65 KB, text/plain
Details
Bug 1715369. Handle the case of a StackingContextHelper inside a deferred transform item with a different asr. r?mstange
48 bytes, text/x-phabricator-request
Details | Review
Bug 1715369. Add test. r?botond,hiro,mstange
48 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:89.0) Gecko/20100101 Firefox/89.0

Steps to reproduce:

  1. Open https://account.protonmail.com/reset-password
  2. Enter some random email address ending in @protonmail.com
  3. Click "Next"
  4. Tick the checkbox
  5. Try to select some of the images

Actual results:

Images cannot be selected.

Expected results:

Images should be able to be selected.

Attached file Troubleshooting info — 

    
    

    
  
The Bugbug bot thinks this bug should belong to the 'Core::DOM: Navigation' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.


Component: Untriaged → DOM: Navigation
Product: Firefox → Core

    
    

    
  
Lyubomir, do you have page zoom enabled?


My co-worker (Peter) wasn't able to reproduce the bug at first. He could click the images successfully at first. But after zooming the page (aka "full zoom" with Ctrl +), image clicking stopped working. After zooming back out to the default zoom level, image clicking still didn't work.


Emilio, this bug might be related to "full zoom", which I am told you worked on recently. Bug 1715187 is a similar issue for pinch-zoom.


Tracking for Fission M7a. This bug is important because it can break websites.


Status: UNCONFIRMED → NEW
Fission Milestone: --- → M7a

          status-firefox89:
          --- → disabled

          status-firefox90:
          --- → disabled

          status-firefox91:
          --- → affected

          status-firefox-esr78:
          --- → disabled
Component: DOM: Navigation → Panning and Zooming
Ever confirmed: true
Flags: needinfo?(emilio)
See Also:  → 1715187

    
    

    
  
I have display scaling for KDE set at 125% on 1920x1080. I don't have zoom.



    
    

    
  
(In reply to Chris Peterson [:cpeterson] from comment #3)




Bug 1715187 is a similar issue for pinch-zoom.




Note that bug 1715187 is happening on an OOP iframe transformed by an ancestor element in ancestor document, which rarely happens.


And in this case, the captcha is inside a <dialog> element which has a transform animation scale:0.8 -> scale:1.0 with fill:both, presumably full-zoom is not related, it looks like a dup of bug 1715187.


As for bug 1715187, I thought I fixed the transformed case, but actually I didn't. :/



    
    

    
  
I don't get any images to tick when I click the hCaptcha thing unfortunately. Do we know if this is actually fission-related? Also, does it work in Firefox 90? I fixed bug 1710059, which looks fairly related. If you can reproduce reliably, can you test a nightly build and see if it works there? Thank you.


Flags: needinfo?(emilio) → needinfo?(liubomirwm)

    
    

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




I don't get any images to tick when I click the hCaptcha thing unfortunately.




Same. (I do get the images in Tor Browser, but... no Fission there, and it's based on 78.11esr.)



    
    

    
  
The images only appear sometimes. hCaptcha uses undisclosed heuristics to decide whether further verification is required. Sometimes multiple attempts or attempting from a private browsing window are enough to eventually trigger it.



    
    

    
  
I can reproduce without any pinch zooming or full zooming. Using a vpn seems to be enough to trigger the images to show up almost 100% of the time. Fission seems to be required to reproduce.



    
  
Flags: needinfo?(liubomirwm)

    
    

    
  
Latest nightly is still broken. Regression ranging this I get the enablement of webrender for my machine. If I pref on webrender and fission I can't get a usable regression range as the captcha popup is broken even before getting to the images and/or the fission pref doesn't exist yet. I got lost somewhere between 2019-04 and 2019-08 I think.



    
  
See Also: 1715187 → 

    
    

    
  
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)




And in this case, the captcha is inside a <dialog> element which has a transform animation scale:0.8 -> scale:1.0 with fill:both, presumably full-zoom is not related, it looks like a dup of bug 1715187.




Oh, forgot about this comment. This does seem to be part of it, removing these animation styles from the dialog does fix the problem, so maybe related to bug 1715187 after all.



    
    

    
  
I think I understand what the problem is.  For hit-testing in OOP iframe APZ collects transform values here in HitTestingNode::GetTransformToGecko, but transform values animated by CSS animations are not collected at all, we collect a snapshot animating value when we paint the animation on the main-thread there, so it would be a wrong transform.



    
    

    
  
And TBH, I don't think we can fix this within the M7a time span.



    
  
See Also:  → 1715187

    
    

    
  
Removing the css animation from the dialog using devtools fixes the problem.


I tried adding the same animation to https://hsivonen.fi/fission-host.html but I couldn't reproduce the problem there.


Looking at the page in devtools I couldn't find any other property that caused the problem. The only thing I found is that there are two nested iframes, and I'm assuming they are both oop.



    
    

    
  
Replacing the animation with transform: scale(1) and I can still reproduce the problem (removing the animation and not adding that transform and it works of course).



    
  
Assignee: nobody → tnikkel

    
    

    
  
Not sure why I didn't notice earlier but we are hitting the "Two layers that scroll together have different ancestor transforms" assert. They are both just translations, one if 0 170 (the content area offset), the other is 888 315, that sort of but not actually equal to an offset of a containing transformed element. So something is causing out offset calculation to get confused, maybe the extra iframe?


Severity: -- → S2
Priority: -- → P1

    
    

    
  
The offset we compute for the captcha oop iframe is to the dialog element (because it is considered transformed). So we are relying on the ancestor transform to have the translation from the dialog element to the top left of the root content document. But there is no other node in the hit testing tree that is an ancestor of that oop iframe's hit testing tree node that has that translate (or any transform besides the chrome offset). The way that webrender does the ancestor transforms is somewhat complicated, so I expect the bug is in there. Next step is to dig in to that.



    
    

    
  
I think I know what's going wrong here now. The page structure is


<dialog element that has an animated transform (animation is done, so it's not actually transformed, but we have to treat it as transformed>

<some div that scrolls>

<in process iframe>

<oop iframe>


When we are CreatingWebrenderCommands for the dialog we creata new StackingContextHelper and set the mDeferredTransformItem on it. The transform on this item needs to be incorporating into everything until we pop this StackingContextHelper.


Later we enter the inprocess iframe, this creates another StackingContextHelper. At the end of it's constructor we handle propagating the mDeferredTransformItem down into us


https://searchfox.org/mozilla-central/rev/fc95c6ad297d9d257f05599d01741503f3f57326/gfx/layers/wr/StackingContextHelper.cpp#100


But because our asr is different from the transform items asr (see the <some div that scrolls> above between the transform and inprocess iframe) we don't pull in the parent item or transform. So everything inside the in process iframe does not know of the existence of the transform. This seems wrong to me: if the stacking context helper wasn't there, (say it was a scrolling div or something and not an iframe) then we'd still keep the mDeferredTransformItem around even though the asr changed.


Removing the asr condition from StackingContextHelper fixes the bug but seems to cause a couple of failures on try. I'll have to look into those, they don't immediately look obvious why.



    
    

    
  
(In reply to Timothy Nikkel (:tnikkel) from comment #18)




Removing the asr condition from StackingContextHelper fixes the bug but seems to cause a couple of failures on try. I'll have to look into those, they don't immediately look obvious why.




Looking at the history a bit, this patch suggests that, at least at the time, a different mechanism was intended to handle the case where the ASR changes: https://phabricator.services.mozilla.com/D8111



    
    

    
  
(In reply to Botond Ballo [:botond] from comment #20)




(In reply to Timothy Nikkel (:tnikkel) from comment #18)




Removing the asr condition from StackingContextHelper fixes the bug but seems to cause a couple of failures on try. I'll have to look into those, they don't immediately look obvious why.




Looking at the history a bit, this patch suggests that, at least at the time, a different mechanism was intended to handle the case where the ASR changes: https://phabricator.services.mozilla.com/D8111




Thanks for finding that. That seems like it would handle the case where the new stacking context helper was for a transform item, but not for any other type of stacking context helper. The tests that are failing might be the ones added in that patch, which is good to know.



    
    

    
  
(In reply to Timothy Nikkel (:tnikkel) from comment #21)




(In reply to Botond Ballo [:botond] from comment #20)




(In reply to Timothy Nikkel (:tnikkel) from comment #18)




Removing the asr condition from StackingContextHelper fixes the bug but seems to cause a couple of failures on try. I'll have to look into those, they don't immediately look obvious why.




Looking at the history a bit, this patch suggests that, at least at the time, a different mechanism was intended to handle the case where the ASR changes: https://phabricator.services.mozilla.com/D8111




Thanks for finding that. That seems like it would handle the case where the new stacking context helper was for a transform item, but not for any other type of stacking context helper. The tests that are failing might be the ones added in that patch, which is good to know.




I think now maybe that the right fix is to just change s/transform item/any item that creates a stacking context helper/ in that patch. I'll try that next.



    
    

    
  
(In reply to Timothy Nikkel (:tnikkel) from comment #22)




(In reply to Timothy Nikkel (:tnikkel) from comment #21)




(In reply to Botond Ballo [:botond] from comment #20)




(In reply to Timothy Nikkel (:tnikkel) from comment #18)




Removing the asr condition from StackingContextHelper fixes the bug but seems to cause a couple of failures on try. I'll have to look into those, they don't immediately look obvious why.




Looking at the history a bit, this patch suggests that, at least at the time, a different mechanism was intended to handle the case where the ASR changes: https://phabricator.services.mozilla.com/D8111




Thanks for finding that. That seems like it would handle the case where the new stacking context helper was for a transform item, but not for any other type of stacking context helper. The tests that are failing might be the ones added in that patch, which is good to know.




I think now maybe that the right fix is to just change s/transform item/any item that creates a stacking context helper/ in that patch. I'll try that next.




This works and seems to pass try server. I'll get a patch for that up.



        Attachment #9228946 -
        Attachment description: Bug 1715369 mDeferredTransformItem's need to be propagated down through StackingContextHelper's even if the asr changes. r?mstange → Bug 1715369. Handle the case of a StackingContextHelper inside a deferred transform item with a different asr. r?mstange

    
    

    
  
A testcase is pretty easy to create now that I've debugged it:


<!--transform

asr change

in process iframe/stackingcontext

oopif-->

<div style="height: 300px; width: 400px; transform: scale(1.1); transform-origin: top left;">

<div style="overflow: scroll; height: 200px;">

<iframe style="border: 10px;" frameborder="10" src="fif.html"></iframe>

<div style="height: 200vh;"></div>

</div>

</div>


fif.html contains

<iframe style="border: 10px;" frameborder="10" src="https://hsivonen.fi/fission-host.html"></iframe>



    
    

    
  
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9361677296f5
Handle the case of a StackingContextHelper inside a deferred transform item with a different asr. r=mstange
https://hg.mozilla.org/integration/autoland/rev/c9cb7876d030
Add test. r=hiro

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/9361677296f5

https://hg.mozilla.org/mozilla-central/rev/c9cb7876d030


Status: NEW → RESOLVED
Closed: 3 years ago

          status-firefox91:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: