Closed Bug 770626 Opened 12 years ago Closed 12 years ago

'Back' gesture on Mac magic mouse/Macbook trackpad triggers infinite loop of unbeforeunload popups

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19
Tracking Flags:
Tracking Status
firefox15 - ---
firefox16 - ---
firefox17 - wontfix
firefox18 - ---
firefox19 - ---

People

(Reporter: shanid.sv, Assigned: smichaud)

Details

(Keywords: qawanted)

Attachments

(4 files)

Testcase from comment #5
290 bytes, text/html
Details
Fix with logging
2.33 KB, text/plain
Details
Proposed Cocoa-level fix
487 bytes, patch
smichaud
: review-
Details | Diff | Splinter Review
Alternate patch that doesn't interfere with swipe animation
3.71 KB, patch
BenWa
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20100101 Firefox/13.0.1
Build ID: 20120614114901

Steps to reproduce:

User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:13.0) Gecko/20100101 Firefox/13.0.1

1) Open any website, say www.google.com

2) Through firebug console, attach onbeforeunload event 
window.onbeforeunload = function(){
    return "some string";
};

3) Go back to previous page using back gesture on magic mouse[swipe one finger from right to left on magic mouse] or Macbook trackpad[swipe two fingers from right to left on trackpad]


Actual results:

Triggers an infinite loop of onbeforeunload confirmation popups - "This page is asking you to confirm that you want to leave - data you have entered may not be saved" 
The only way to exit the loop is to force quit the browser. 


Expected results:

1) Selecting "Leave Page" option should load the previous page.
1) Selecting "Stay on Page" option should close the confirmation popup.
OS: Windows XP → Mac OS X
Can you do this without Firebug? Firefox has a Web Developer console which should allow you to test this. Also, please try in the latest nightly (nightly.mozilla.org).

Thanks
I can replicate the issue in the latest nightly release. Tested using Firefox Web Developer as per your instruction.
I used nightly version # 16.0a1 (2012-07-10). on MAC OS X 10.7.4.
Okay, thanks. Can you please retest with a brand new profile on Nightly? 
http://kb.mozillazine.org/Profile_Manager

This will eliminate your profile as a cause.
I was able to replicate this on one of our consultant's machines. It was happening when she swiped ONLY and not with any other method of browsing back (backspace, Option+back, back button). 
FF 13.0.1 on MAC OS X 10.7.2

Here is the simplest test case:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html>
    <head>
        <title>Test Case for Swipe and onbeforeunload</title>
    </head>

    <body onbeforeunload='return "test"'>
        try swiping back
    </body>
</html>
I can confirm this using my 2009 MBP with the testcase in Comment 5. I was able to reproduce with the latest nightly as well as FF 13.0.1.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Untriaged → Event Handling
Product: Firefox → Core
Version: 13 Branch → Trunk
No need to track for FF15 - this is not a new regression (and Mac gestures have been in Firefox for months now). That's not to say we wouldn't take a fix, but it's not release critical.

If people are running into this frequently, or its affecting a large group of users, we'll reconsider. Please re-nominate for tracking in that case.
tracking-firefox15: ? → -
Keywords: qawanted, regressionwindow-wanted
Marcia, given you are able to reproduce this, can you please work on a regression range?
Any movement on this?
This is a serious bug for a segment of users on our application.  We have kindergarteners using our application.  They're only 5-6 years old, so their fine motor skills aren't well developed, and they end up accidentally swiping back often.  At that point Firefox has to be force-quit, which requires teacher intervention, and they lose their progress inside of our application.

Can we get this re-prioritized?  I'm not familiar with the Firefox tracking system.  It looks like I have the option to set '?' for Firefox 16, so I did that.  Let me know if that's not the correct way to renominate.
tracking-firefox16: --- → ?
This also happens with the Back button, so it has nothing to do with the swipe gesture.

Someone needs to test on Windows and/or Linux.
> This also happens with the Back button, so it has nothing to do with
> the swipe gesture.

Oops, this is incorrect.  When you use the Back	button the Stay	on
Page and Leave Page buttons work correctly.

And this problem doesn't happen	on Windows (testing with the Back
button).
I could go either way on blocking for this bug.

We can't block on FF 16, since that's already been released.  And I don't think we should block on FF 17.  FF 18 then (if we decide to block)?

Clearly not many people are seeing this bug (otherwise we'd have a lot more reports).  But those who do see it are badly inconvenienced.

I don't know how difficult it would be to fix this bug.  I also don't know how difficult it'd be for web page authors to work around it.
(In reply to Steven Michaud from comment #11)
> This also happens with the Back button, so it has nothing to do with the
> swipe gesture.
> 
> Someone needs to test on Windows and/or Linux.

I have not been able to replicate it on Windows with any of the methods of going back (backspace, back button, back button on the mouse, alt+leftArrow). On a Mac it was only replicatable by the swipe, and not by any other methods.

(In reply to Steven Michaud from comment #14)
> I could go either way on blocking for this bug.
> 
> Clearly not many people are seeing this bug (otherwise we'd have a lot more
> reports).  But those who do see it are badly inconvenienced.
> 
> I don't know how difficult it would be to fix this bug.  I also don't know
> how difficult it'd be for web page authors to work around it.

Not sure about fixing (I hope it would not be hard though.) It's fairly difficult to work around it. We cannot detect that it's a swipe vs any other "page leave action". And we do need to be able to warn a user if they try to leave before a save. 

Just as a data point this same technique is used on various large webapps like Gmail and others.
I took a peek at the code involved, and saw that a swipe back triggers the same browser function as a back button click, Browser:BackOrBackDuplicate.

Given that, it seemed like what was happening was that multiple swipes were being sent, not that the code inside the back function or the code that handles window.onbeforeunload was working differently than intended.

I tested this by making a page that makes use of the window.onbeforeunload event.  I swiped back, and got what seemed like an infinite series of Leave/Stay dialogs.  However, I kept clicking through them, and eventually got through after about thirty clicks.  This number is pretty consistent, though is sometimes less and sometimes greater than 30.

So, it seems that what should be a single swipe is being interpreted as multiple swipes.  The touch length threshold gets met, the Browser:BackOrBackDuplicate action is triggered, and the Javascript window.onbeforeunload event fires.  The user makes a selection, but then there still touch data in a buffer left to be processed.  This repeats until there is no longer enough data in the buffer to constitue a swipe.

Does that make sense?
> Does that make sense?

Yes.  But you'll need to add logging to various places in the code to find out whether or not it's true.

I'd start with calls to NSLog() in the relevant parts of nsChildView.mm.
Thanks for pointing me to the right place.

I added some logging and found that multiple swipes of identical magnitude were being processed whenever I swiped back with a handler attached to window.onbeforeunload.  These looked like this:

11/3/12 3:55:47.207 PM firefox[22059]: [3BE807A1-ECF8-4A53-AE46-5B353C48A71A-22059-000182612364922B] starting new block
11/3/12 3:55:47.207 PM firefox[22059]: [3BE807A1-ECF8-4A53-AE46-5B353C48A71A-22059-000182612364922B] got swipe with gestureAmount value 0.48
11/3/12 3:55:47.214 PM firefox[22059]: [59E8BCAD-C68F-4001-88E0-B7161AC15E93-22059-0001826123D59D38] starting new block
11/3/12 3:55:47.215 PM firefox[22059]: [59E8BCAD-C68F-4001-88E0-B7161AC15E93-22059-0001826123D59D38] got swipe with gestureAmount value 0.48
11/3/12 3:55:47.219 PM firefox[22059]: [0608EF2F-FD5C-4C30-9E43-A810DBC099ED-22059-000182612422EC54] starting new block
11/3/12 3:55:47.220 PM firefox[22059]: [0608EF2F-FD5C-4C30-9E43-A810DBC099ED-22059-000182612422EC54] got swipe with gestureAmount value 0.48
11/3/12 3:55:47.225 PM firefox[22059]: [97A3C5DE-4F88-453F-B5FD-8B79B4CC8B9B-22059-0001826124731859] starting new block
11/3/12 3:55:47.225 PM firefox[22059]: [97A3C5DE-4F88-453F-B5FD-8B79B4CC8B9B-22059-0001826124731859] got swipe with gestureAmount value 0.48

...and so on.  The first part of the log in brackets identifies the block instance, I wanted to rule out the possibility that two instances of the block that handles gesture were running simultaneously.  It looks like that's not the case, these all come in serially.

The number of swipes recorded in the logs corresponds perfectly to the amount of clicks required to clear the dialog.

I could see in the block that swipes aren't processed in case animationCancelled is YES.  I had an idea that maybe this doesn't happen in case the browser has to process onbeforeunload.  I set animationCancelled to YES before the call to mGeckoChild->DispatchWindowEvent(geckoEvent), and this appears to have mitigated the problem without any side effects.  I can swipe back on a page with onbeforeunload bound and only get a single dialog, and I can swipe forward and back between pages the same as before.

I'm not sure if this is the right place for a fix.  On one hand, it seems like this fix should occur in the code that handles onbeforeunload.  On the other, this bug apparently only affects the Mac build, and fixing just the Cocoa code keeps the testing profile much lower.

I'm attaching two diffs.  One with the logging code and the fix, the other with just the fix, which is one line.

I also openly acknowledge that I'm not at all familiar with this code, or Objective-C, and don't know if manually setting animationCancelled could have any adverse effects.
Attached file Fix with logging — 

    
    

    
  


  

        Attachment #678070 -
        Flags: review?(smichaud)

    
    

    
  
Comment on attachment 678070 [details] [diff] [review]
Proposed Cocoa-level fix

This works fine in current code, but would mess up what we're attempting to do in bug 678392.

Current code doesn't do anything after the block in -[ChildView maybeTrackScrollEventAsSwipe:...] is called (by the OS) with 'phase' set to NSEventPhaseEnded.  So it doesn't matter (in current code) if (as per your patch) we cancel all further calls to the block.  But we need to see these calls if we're doing swipe animation.

Starting from what you found, I was able to find an equally simple fix that doesn't cancel animation.  I'll post it in my next comment.

Thanks for your work on this!  It gave me the clues I needed to realize what sort of problem this bug is, and that it should be quite easy to fix.

        Attachment #678070 -
        Flags: review?(smichaud) → review-

    
    

    
  


  
Here's my patch.  It's explained in a code comment.

I've started a tryserver build, which should be available in a few hours.  Once that happens, I'd like the people who've seen this bug to test with it.
Assignee: nobody → smichaud

    
    

    
  
Excellent news, thank you!

Would this be on track for Firefox 17?  We'd like to communicate with our customers that are impacted by this bug that a fix is on the way, but want to make sure that we're accurate on when the fix will be available.

    
    

    
  
> Would this be on track for Firefox 17?

Probably not.  FF 17 is (I think) due out later this month, and I suspect that's too close for comfort.

We'd also have to accelerate this patch's progress to get it into FF 18.  But the patch is very simple, and unlikely to cause trouble -- so I don't think that'll be a problem.

Before anything else, though, we need to get this patch on the trunk and let it bake there for a while.

    
    

    
  
We should get this to FF17 at some point, at least when FF17 becomes ESR.

          tracking-firefox17:
          --- → ?

          tracking-firefox18:
          --- → ?

          tracking-firefox19:
          --- → ?

        Attachment #678914 -
        Flags: review?(bgirard)

        Attachment #678914 -
        Flags: review?(bgirard) → review+

    
    

    
  
Comment on attachment 678914 [details] [diff] [review]
Alternate patch that doesn't interfere with swipe animation

Landed on mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d105eace108b

    
    

    
  
As Alex pointed out in comment 7 this is not a regression or a release critical issue. We're past the point of taking fixes on the 17 branch that we wouldn't chemspill for but please go ahead with nominating for uplift to Aurora once qa verifies the fix on trunk. If the risk is low, it will get proper bake time on 18 before shipping.

          status-firefox17:
          --- → wontfix

          tracking-firefox16:
          ? → -

          tracking-firefox17:
          ? → -

          tracking-firefox18:
          ? → -

          tracking-firefox19:
          ? → -
Keywords: qawanted, 
          
            verifyme

    
    

    
  
Marcia, can you please verify this once the fix has landed on mozilla-central?
QA Contact: mozillamarcia.knous

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/d105eace108b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

    
    

    
  
I don't know if this is still relevant, but the bug seems to be fixed in the latest nightly release. I tested it on the same machine as the one where I could originally reproduce the bug.

Thanks all.

    
    

    
  
This bug may partially regress due to the patches for bug 678392 should the feature be turned on before bug 673875 is checked in. Although there will not be an infinite loop of onbeforeunload events as reported in this bug, pressing the "stay on page" button will fail to tear down the animation overlay. A fix will be checked in as part of the patches for bug 673875.

    
    

    
  
>     1.46 +          // Probably because of bug 478703, this really messes things up,

What bug number was that supposed to be?

    
    

    
  
Oops, that should have been "bug 478073".

    
    

    
  
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
Component: Event Handling → User events and focus handling

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: