vsireesha246 Reporter
	Description • 10 years ago

Steps to Reproduce:

Open Browser->Click on '+' icon continuously to open more than 20 tabs

Check the '<' back button,stops working.

This issue is reproduciable in Version v2.0

	Wayne Chang [:wchang]
	Comment 1 • 10 years ago

Mike,

please also help with this repro on flame, thanks a lot!

	Mike Lien[:mlien]
	Comment 2 • 10 years ago

I cannot reproduce this stop working issue on the latest v2.0 with Flame
But continously click '+' to open many new tabs is a legacy bug, yuo can refer to bug1011669

Gaia      d056733f8a7a1a152f5458b323f092c47dbffa48
Gecko     https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/03ca6b835967
BuildID   20140903160204
Version   32.0

	vsireesha246 Reporter
	Comment 3 • 10 years ago

Hi Mike and Ben,

I can reproduce this issue by pressing the '+' new tab button very fastly and wait upto more than 20 to 40 tabs it opens..

Then back button stops working..sometimes it is taking too much time to respond for click on back button.
But i can able to click settings button and '+' buttons.

Here the problem with back button always.

	Wayne Chang [:wchang]
	Comment 4 • 10 years ago

(In reply to vsireesha246 from comment #3)
> I can reproduce this issue by pressing the '+' new tab button very fastly
> and wait upto more than 20 to 40 tabs it opens..
> 
The '+' button is not for you to use that way. Users don't open 20~40 _blank_ tabs and the designed UX doesn't flow that way either.

> Then back button stops working..sometimes it is taking too much time to
> respond for click on back button.
> But i can able to click settings button and '+' buttons.
> 
> Here the problem with back button always.
This didn't happen for me on a flame, please check it on flame and provide mozilla build information.

	yifan [:yifan][:yliao] Assignee
	Comment 5 • 10 years ago

This could be reproduced on flame. It seems that adding multiple empty tabs is not allowed by the code, but repeatedly tapping the '+' button will somehow bypass the check. This will hang the browser app eventually.

Gaia      91dd0e596aa7c124dd968e1474b23e7992dc35a1
Gecko     https://hg.mozilla.org/mozilla-central/rev/37ac55a26014
BuildID   20140817160201
Version   34.0a1
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014

	vsireesha246 Reporter
	Comment 6 • 10 years ago

Thanks You Yliao, helping us to reproduce this issue.


(In reply to Yi-Fan Liao [:yifan][:yliao] from comment #5)
> This could be reproduced on flame. It seems that adding multiple empty tabs
> is not allowed by the code, but repeatedly tapping the '+' button will
> somehow bypass the check. This will hang the browser app eventually.
> 
> Gaia      91dd0e596aa7c124dd968e1474b23e7992dc35a1
> Gecko     https://hg.mozilla.org/mozilla-central/rev/37ac55a26014
> BuildID   20140817160201
> Version   34.0a1
> ro.build.version.incremental=110
> ro.build.date=Fri Jun 27 15:57:58 CST 2014

	vsireesha246 Reporter
	Comment 7 • 10 years ago

Root cause for this issue may be the memory abort error.Before this error the current tab is null and after that Gecko is throwing the memory abort error and pipe broken exceptions.

	vsireesha246 Reporter
	Comment 8 • 10 years ago

As per the code analysis once the user presses the '+' icon it will try to create new tab and if the user will not provide any URL then its try to delete the newly created tab.

Due to fast clicking the '+' icon might be causing the creating and deleting operations and some how we are referencing the deleted tab causing this issue.

	vsireesha246 Reporter
	Comment 10 • 10 years ago

Please also see: https://bugzilla.mozilla.org/show_bug.cgi?id=1011669#c9

	vsireesha246 Reporter
	Comment 11 • 10 years ago

I added just null check on top of the Bug-1011669 patch and after that i can't able to reproduce this issue.

I don't know the exact reason why the this.currentTab or tab are undefined once this issue got reproduced after applying the patch BUG-1011669.

Thanks..
Sireesha

	Ben Francis [:benfrancis]
	Comment 12 • 10 years ago

Comment on attachment 8493667 [details] [diff] [review]
Bug_1059650.patch

vsireesha246, thanks for the patch.

I'm a bit concerned that you're not sure why this patch fixes the issue you're seeing, and I'm also not convinced it isn't just symptomatic of an Out of Memory state where the clearing of session history (and therefore the back button not working) is an expected outcome.

The use case described in the bug obviously isn't one which would be carried out during normal use.

I would feel happier if you could show more conclusively that this patch actually fixes the bug you're seeing, either through tests or evidence of more thorough manual testing.

This may just be a duplicate of bug 1038435

Thanks

	vsireesha246 Reporter
	Comment 13 • 10 years ago

This issue happens due to tap event is triggered on tabsSwipeMngr instead of screenSwipeMngr.

In this case this.id=NULL.

https://github.com/mozilla-b2g/gaia/blob/v2.0/apps/browser/js/browser.js#L1633

	vsireesha246 Reporter
	Comment 14 • 10 years ago

Hi Ben & Vchen,

Would you please help me on this issue.
Its happening sometimes very frequently even after adding the patch bug 1038435.

Thanks..
Sireesha

	Rachelle Yang [:ryang][ryang@mozilla.com]
	Comment 15 • 10 years ago

Hi Vsireesha, with patch of Bug1038435, is the behaviour changed or the issue still the same?
Since I am very confused with your description " sometimes very frequently ...."

	vsireesha246 Reporter
	Comment 16 • 10 years ago

Hi Rachelle,

As mentioned in comment12 i feel this bug is not duplicate of Bug 1038435.

This issue occurs sometimes very frequently and sometimes its very rare to reproduce this issue.
As per my analysis in comment13,the 'tap' event is going to close 'x' button handler causing this problem.

Thanks,
Sireesha

	Rachelle Yang [:ryang][ryang@mozilla.com]
	Comment 17 • 10 years ago

Hi Ben, Could you please kindly help us check this issue again ? 

Looks like the patch on bug 1038435 doesn't completely solve the issue.

Could you please check it ? Thank you very much.

	yifan [:yifan][:yliao] Assignee
	Comment 18 • 10 years ago

Could it be reproduced after applying bug 1011669? I tried with that fix and couldn't reproduce it.

	Chrys Choi
	Comment 20 • 10 years ago

Hi Rachelle,

I have shared easily steps to reproduce:

Sept 1. Open Browser
Sept 2. Click on "1>" icon
Sept 3. Twice fast click on "+" icon
Sept 4. immediately click on "<" icon after 2nd click on "+" icon
Sept 5. Click on "X" icon

And then, "<" icon is not working.

Plz refer to attached video clip.

	yifan [:yifan][:yliao] Assignee
	Comment 21 • 10 years ago

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1075333#c11 and apply the patch for bug 1011669.

	vsireesha246 Reporter
	Comment 22 • 10 years ago

Hi Yliao,

The Patch of Bug-1011669 is already applied ans still this issue is reproducible.
As per your comment21 the reason for the this bug and Bug-1075333 is it same.
Which is related to 'transitionend' event not occuring?

Thanks..
Sireesha

	yifan [:yifan][:yliao] Assignee
	Comment 23 • 10 years ago

(In reply to Chrys Choi from comment #20)
> Hi Rachelle,
> 
> I have shared easily steps to reproduce:
> 
> Sept 1. Open Browser
> Sept 2. Click on "1>" icon
> Sept 3. Twice fast click on "+" icon
> Sept 4. immediately click on "<" icon after 2nd click on "+" icon
> Sept 5. Click on "X" icon
> 
> And then, "<" icon is not working.
> 
> Plz refer to attached video clip.

After applying the patch for bug 1011669, you couldn't create more than one empty tab. How do you reproduce this bug after applying the patch?

	vsireesha246 Reporter
	Comment 24 • 10 years ago

Hi Yalio,

After applying Bug-1011669 patch we can't able to open more than one empty tab.Yes you are right.
But the issue is reproducible by clicking multiple times fastly on the '+' icon.
And also please refer the above comment16.

Thanks..
Sireesha

	yifan [:yifan][:yliao] Assignee
	Comment 25 • 10 years ago

Haven't been able to reproduce it. Since tapping the '+' button repeatedly is not a common user behavior and the bug is rare even by doing this, I'd suggest we fix it when there's an easier way to reproduce it.

	vsireesha246 Reporter
	Comment 26 • 10 years ago

This issue also causes below error in Gaia Browser app and user can't able to select any of the Topsites list or Bookmarks list or History list in awesomescreen screen.He user can navigate between topsites,Bookmark and history tabs and close button stops working.

GeckoConsole: [JavaScript Error: "TypeError: Browser.currentTab is undefined" {file: "app://browser.gaiamobile.org/js/awesomescreen.js" line: 87}]

	Doyun Kim
	Comment 27 • 10 years ago

this is blocking issue.

	yifan [:yifan][:yliao] Assignee
	Comment 28 • 10 years ago

When the problem appears, 'tap' events are not fired by 'screenSwipeMngr'. Touch events are still handled by 'screenSwipeMngr' but it doesn't respond to the browser app. Probably something wrong in gesture_detector.js. Working on it.

	Keven Kuo [:kkuo]
	Comment 29 • 10 years ago

[Blocking Requested - why for this release]: partner launch blocker

	yifan [:yifan][:yliao] Assignee
	Comment 30 • 10 years ago

'touchend' event isn't fired occasionally when rapidly tapping at the same spot. This breaks the states in the gesture_detector.js.

Hi Brain, is it related to bug 1075333? Or do you know anyone who could help? Thank you!

	yifan [:yifan][:yliao] Assignee
	Comment 31 • 10 years ago

Hi Viral,

Do you have any idea about comment 30? Thank you!

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 32 • 10 years ago

(In reply to yifan [:yifan][:yliao] from comment #30)
> 'touchend' event isn't fired occasionally when rapidly tapping at the same
> spot. This breaks the states in the gesture_detector.js.
> 
> Hi Brain, is it related to bug 1075333? Or do you know anyone who could
> help? Thank you!

If it's touchend that isn't fired, then I guess not (that bug is about transitionend).

	Rachelle Yang [:ryang][ryang@mozilla.com]
	Comment 33 • 10 years ago

Confirmed as partner launch blocker.

	viral [:viralwang]
	Comment 34 • 10 years ago

I think it may relative to apz, can we still reproduce it if we disable apz?

	Rachelle Yang [:ryang][ryang@mozilla.com]
	Comment 35 • 10 years ago

Hi Vsireesha, 

Could you please kindly check if this issue wont happen with APZ disabled from settings ?
Thanks you very much!

	Doyun Kim
	Comment 36 • 10 years ago

Hi Rachelle,

It's reproduced easily even with APZ disabled.

Thanks,

	Doyun Kim
	Comment 37 • 10 years ago

Hi Rachelle,

It's reproduced easily even with APZ disabled.

Thanks,

	yifan [:yifan][:yliao] Assignee
	Comment 38 • 10 years ago

Hi Viral,

It can still be reproduced when APZ disabled.

	Ben Francis [:benfrancis]
	Comment 39 • 10 years ago

Thank you for the video Doyun Kim, I understand the bug report better now. I thought the bug was talking about the browser back button, but it's actually the "<" button to close the tabs view. That means bug 1038435 is definitely not related, sorry for that misunderstanding.

I'm disappointed bug 1011669 doesn't help with this.

Thank you yifan for investigating, so it looks like it might be shared code in gesture_detector.js which is getting confused when you tap the screen very quickly.

Brian, David, do you think this is most likely to be a Gecko or Gonk bug with touchend events getting dropped, or is it a bug in gesture detector getting into a broken state? I'm afraid I'm not familiar with the gesture_detector.js code.

	Cody Roesch [:croesch]
	Comment 40 • 10 years ago

Removing QAWanted tag as the answers in comments 36, 37, 38 looks to have satisfied the QAWanted request.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 41 • 10 years ago

(In reply to Ben Francis [:benfrancis] from comment #39)
> Brian, David, do you think this is most likely to be a Gecko or Gonk bug
> with touchend events getting dropped, or is it a bug in gesture detector
> getting into a broken state? I'm afraid I'm not familiar with the
> gesture_detector.js code.

I'm afraid I don't know anything about touchend events. As for missing transitionend events, I've been unable to reproduce that on desktop so I'm currently trying to build 2.0 to see if I can reproduce it for bug 1075333.

	David Flanagan [:djf]
	Comment 42 • 10 years ago

(In reply to Ben Francis [:benfrancis] from comment #39)
> Brian, David, do you think this is most likely to be a Gecko or Gonk bug
> with touchend events getting dropped, or is it a bug in gesture detector
> getting into a broken state? I'm afraid I'm not familiar with the
> gesture_detector.js code.

I haven't looked at gesture detector in quite some time. If touchend events are not happening that seems pretty clearly like a gecko bug to me. It may be that there is some simple workaround we could do in gesture detector to compensate, however.

What's the evidence implicating touchend events or gesture detector? I see logging code attached at comment #30. But I don't see a log attached showing that the events get lost.  

I forget whether gesture detector listens for touchcancel events or not. Those are part of the spec, though I've never seen them sent. I think they're supposed to happen if a touch slides off the edge, so perhaps we're missing a touchend event because a touchcancel is being sent instead.

I'm out of time right now, but leaving the needinfo flag set, so I remember to come back to this and take another look.

	David Flanagan [:djf]
	Comment 43 • 10 years ago

I've looked into this some more and it has nothing to do with gesture detector. If you put a console.log statement in screenSwipeMngr.tap(), you can see that the tap event is indeed being delivered correctly when the user taps on the back button.  And showPageScreen() is being called. The problem is that this.currentTab is undefined show showPageScreen() throws an exception.

Getting the browser into this state can be a tricky. The STR I'm using:

1) Launch browser

2) Tap on "1>" to go to new tabs screen

3) Double-tap on the + icon. This will open two new tabs before the awesomescreen slides in.

4) Now dismiss the awesomescreen by tapping X

5) You should see two blank new tabs now.  Click the X on the bottom one to dismiss it. This may take multiple tries, and you should see an error message about this.currentTab not define, coming from an exception in selectTab (line 1292).

6) At this point, the back button will no longer work, because this.currentTab is undefined (exception at line 1390)

It is easy to get to step 5 and see two blank new tabs, with a just slightly faster than normal double-tap. But the rest of the bug doesn't always reproduce when you get there. Sometimes you can dismiss the second tab and go back with no problem. I don't know if it is intermittent, or if you just have to do the double-tap super fast (like a QA engineer) to get the app into a state where deleteTab somehow manages to pass selectTab a bogus tab id and break the currentTab property.

I'm not sure what is going wrong to get this.currentTab undefined, but it seems clear to me that the fix here is to prevent the new tab button from responding to multiple taps so quickly so you can't get into this state to begin with.

	David Flanagan [:djf]
	Comment 44 • 10 years ago

I tried adding this code to browser.js:

   handleNewTab: function browserHandleNewTab(e) {
+    if (this.inTransition) {
+      return;
+    }
     this.inTransition = true;

This prevents rapid double clicks on the + button from creating multiple new tabs.  But it isn't enough.  If I just hammer repeatedly on the upper right corner of the screen I still get the multiple open new tabs and can recreate the bug when closing them. I don't know what is going on there, but I see that when I just tap quickly in the upper right I see the new tabs screen, the awesomescreen and the empty tab home screen so something is making us go back to one of the empty tabs even though I'm only ever tapping on >, +, or x buttons.

	David Flanagan [:djf]
	Comment 45 • 10 years ago

I notice that the patch from vsireesha above uses 

if (this.inTransition || this.currentScreen !== this.TABS_SCREEN) {
  return;
}

If I switch to using that, I never get more than one new tab. I don't know why the + button of the tabs screen would be responding to events if it is not the currentScreen.  And I see that with just that patch, then multiple rapid tabs gives "TypeError: tab is undefined" at line 1144.  I think vsireesha's patch has code to prevent that error, but not code to get to the root cause.

I don't really understand what is going on here, but suspect that the fact that rapid taps can take us back to the page screen has something to do with this...  By adding some logging statements, I see that with the patch above, rapid taps in the upper right can cause the screenSwipeMngr.tap() function to be triggered making us go back to the page screen. So maybe screenSwipeMngr needs to check inTransition and do nothing when it is true.

I hate this kind of rapid tap, CSS state transition, data structure state consistency bug, and I'm going to stop investigating new. Good luck with it Ben!

It is pretty easy to reproduce with just a double tap on the +, so I agree that it is worth blocking on. We at least need a partial patch so that the user can't break the browser with just a few taps.  If there is still a bug that can be triggered by rapid hammering in the upper right maybe we don't need to fix that for 2.0...

Perhaps that is enough to solve the multiple tabs issue. I don't know if or why any of the rest of vsireesha's patch would be necessary, though. And I don't know why the new tab button of the tabs screen can even be pressed if

	Ben Francis [:benfrancis]
	Comment 46 • 10 years ago

That's super helpful, thank you David.

So we're back to browser code, let me take a deeper look.

	Ben Francis [:benfrancis]
	Comment 47 • 10 years ago

OK I think I found the underlying cause. There are two CSS transitions during which it is possible to hit the new tab button multiple times if you're very fast. One is the transition into and out of the tabs screen and the other is the new tab animation.

By preventing the new tab button from being used while these transitions are in progress I think we can prevent the broken state. 

inTransition was being set in multiple places which were interacting with each other so I have centralised that logic in the place where the transition is actually triggered.

	vsireesha246 Reporter
	Comment 48 • 10 years ago

Hi Ben,

I tested the patch https://github.com/mozilla-b2g/gaia/pull/26038 in V2.0 and i found the below errors.

After opening more than 20 to 25 new tabs “+’ stops working with this patch.
The root cause for this ‘+’ stops working is due to ‘transitionend’ event is not fired causing below condition to fail.

handleNewTab: function browserHandleNewTab(e) {
    if (this.inTransition || this.creatingTab) {
      return;
   }
If ‘transitionend’ fired case only this.creatingTab will be false.

After applying this patch also sometimes ‘<’back button also stops working.

Please also refer https://bugzilla.mozilla.org/show_bug.cgi?id=1075333

Thanks..
Sireesha

	Ben Francis [:benfrancis]
	Comment 49 • 10 years ago

Hi Sireesha, thanks for testing.

I can also reproduce the new tab button not working by opening 40+ tabs on Flame set to 319MB.

The patch blocks multiple new tabs being created by preventing the "new tab" button from working during CSS transitions, but it does rely on transitionend events to unblock that. Your hypothesis that transitionend events are being dropped in high memory usage conditions makes sense. Brian, have you made any progress on verifying this?

We could try adding arbitrary setTimeouts as a fallback, but I'm reluctant to do that because it will make the code more unpredictable and possibly create new race conditions.

QA, I don't think we've looked for a regression window on this yet? Perhaps that will help identify the cause.

This area of code in the browser app really hasn't changed much in the last year so I'd say either this bug has been present for a long time (since at least 1.3) or it's being caused by a Gecko regression. However, the state management in the browser is certainly not bullet proof so it's possible that subtle changes like bug 991547 or bug 983760 have exposed existing broken state problems.

We still haven't identified why this.currentTab is getting set to null so looking deeper into that might be the next stage. I'm just a bit concerned about any more serious refactoring because of the risk of introducing new regressions in this legacy area of code which doesn't have extensive test coverage and is basically in maintenance mode at this point (this code doesn't exist in 2.1 and 2.2).

	Ben Francis [:benfrancis]
	Comment 50 • 10 years ago

OK, if we can't rely on transitionend events to reliably block multiple new tabs being created then we could just accept that multiple new tabs can be created if the user is very quick, and make sure that doesn't break things.

Creating a new tab is quite an intensive process which involves adding a new mozbrowser iframe to the DOM and creating a new system process. This simple patch just adds a null check to ensure this.currentTab doesn't get set to null in some weird in-between stage where multiple new tabs are in the process of being created. This should also prevent all the errors you're seeing.

I've tested this patch on a 319MB Flame and I'm able to create 60+ tabs and everything seems to keep working. Sireesha, can you confirm?

Brian, I've noticed a lot of Gecko errors in the console when the Flame starts to run out of memory, I don't know whether this means anything to you or is any way significant?

  I/Gecko   (  210): ###!!! [Parent][OnMaybeDequeueOne] Error: Channel error: cannot send/recv

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 51 • 10 years ago

(In reply to Ben Francis [:benfrancis] from comment #49)
> The patch blocks multiple new tabs being created by preventing the "new tab"
> button from working during CSS transitions, but it does rely on
> transitionend events to unblock that. Your hypothesis that transitionend
> events are being dropped in high memory usage conditions makes sense. Brian,
> have you made any progress on verifying this?

I'm working on this in bug 1075333. I have a 2.0 build up and running and plan to look into it later today.

I'll report back here if I find anything interesting.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 52 • 10 years ago

(In reply to Brian Birtles (:birtles) from comment #51)
> I'll report back here if I find anything interesting.

Hi Ben, I've finished looking into bug 1075333. Please have a look at bug 1075333 comment 24. The issue in that case appears to be 'mozbrowsertitlechange' events triggering the tab page to be rebuilt in the middle of the transition and therefore causing the transition to be dropped.

Does that shed any light on this bug?

	Duane Dixon [:ddixon]
	Comment 53 • 10 years ago

Issue DOES occur in the earliest 2.0 Flame build we have access to (Shallow Flash, nightly, 319 MB memory). 

Actual Results: Opening 30+ blank tabs in Browser app disables the back button. 

Environmental Variables:
Device: Flame 2.0
BuildID: 20140904114640
Gaia: 13978cf2230652274969536322378d448fd142a4
Gecko: 2537ab191112
Version: 32.0 (2.0) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Removing 'regression window wanted' keyword because this is not a regression.

	Gregor Wagner [:gwagner]
	Comment 54 • 10 years ago

We need to come to a conclusion here. The original description talks about 20 tabs. It seems we have a fix for this issue?
One issue per bug and if this doesn't work with 30 or 40 tabs lets file a follow up since we might run into another issue.

	Dale Harvey (:daleharvey)
	Comment 55 • 10 years ago

Comment on attachment 8521376 [details]
https://github.com/mozilla-b2g/gaia/pull/26063/files

What causes the call to selectTab, we cant really be putting error detection code around for "some weird in-between stage where multiple new tabs are in the process of being created", without understanding the root cause, things will get messy fast.

It sounds like the titlechanged comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1059650#c52 has started finding the root cause of the problem which could lead to an actual fix.

	Dale Harvey (:daleharvey)
	Comment 56 • 10 years ago

Also seems like it should be possible to create an integration test that clicks on the new tab button a whole bunch of times and makes sure we dont blow up.

	Ben Francis [:benfrancis]
	Comment 57 • 10 years ago

OK, let's go deeper again. Discoveries so far...

selectTab() is being called with a null argument by the tap() method of tabsSwipeMngr. The reason this happens is that tabSwipeMngr relies on receiving a mousedown event to set this.id before the tap event calls selectTab(this.id). Sometimes the tap event is fired without the mousedown event being fired which is how it ends up being null. This can be reproduced by David's STR in comment 43.

This code has been around since June 2012 https://github.com/mozilla-b2g/gaia/commit/6a76a0754f2e2d773a7c832c4e3a0dc903e24444

Mouse events got removed from Gesture Detector in bug 950607, but this was after 2.0 branched so can't be the cause. Also, this bug is intermittent, mousedown events do get fired most of the time.

	Ben Francis [:benfrancis]
	Comment 58 • 10 years ago

I'm sorry but I've spent hours investigating this and I haven't been able to find a fix which goes any deeper than my previous patch.

However, I have discovered that the whole "open 20 tabs" thing is a massive red herring. The bug seems to have nothing to do with how many tabs you have open.

Firstly, there's a much simpler STR to reproduce the underlying bug:
* Open the browser app
* Press the tabs button to open the tab tray
* Press the close button on the only visible tab

Expected:
* The tab closes

Actual:
* The tab sometimes closes and sometimes doesn't

Now repeat this and watch the console. Keep pressing the close button until you see the "this.currentTab is null" error in the console. Now try to press the "<" button. It won't work.

What's going on here is that the code to close a tab relies on both a mousedown event and a tap event to be fired so that both the mousedown() and tap() methods of tabSwipeMngr are called in turn. When both of these methods are called everything seems to work fine. However, sometimes only one of the methods gets called (presumably because the event doesn't fire) and this is what causes problems.

If tap() gets called without mousedown() then this.id is null and nothing happens. This can also have the knock-on effect of Browser.selectTab() being called with a null argument which leads to this.currentTab being null which is what causes the "this.currentTab is null" error when you press the "<" button.

There's code in screenSwipeMngr which also depends on both mousedown and tap events and seems to behave unpredictably as a result.

Note that the code I'm talking about in the browser app hasn't changed in about two years, so if there is a regression since before 2.0 then it's probably more likely to be in Gecko. The patch I submitted previously will not make the tab close button always work, but it will prevent the browser getting into a broken state where the "<" button won't work because this.currentTab is null.

Unfortunately I'm now on PTO for a week so if a new Gaia patch is needed someone else is going to have to take this bug from me. I'm sorry about that.

	Ben Francis [:benfrancis]
	Comment 59 • 10 years ago

I just reproduced this bug on 1.4

	Ben Francis [:benfrancis]
	Comment 60 • 10 years ago

Just a guess here, but might this be more reliable if we switched mousedown for touchstart?

	Dale Harvey (:daleharvey)
	Comment 61 • 10 years ago

Ok, will steal while bens away

	yifan [:yifan][:yliao] Assignee
	Comment 62 • 10 years ago

Thank you Ben for providing the deep investigation. I noticed that 'mousedown' event is no longer received by GestureDetector and the 'mousedown' handler in tabSwipeMngr in browser.js no longer works. This commit moves the tab ID selection directly in the 'tap' event. This probably also fix other relative browser bugs with 'this.currentTab.id is null' problem. Please see if the fix helps, thanks!

	Dale Harvey (:daleharvey)
	Comment 63 • 10 years ago

Sorry for the delay, I need to reflash my phone to get on to this version, will do by end of day

	Dale Harvey (:daleharvey)
	Comment 64 • 10 years ago

Comment on attachment 8524481 [details] [review]
pull request for v2.0

This made sense but needs a bit more work, made a comment on github, once that is addressed you can give me review again, thanks

	yifan [:yifan][:yliao] Assignee
	Comment 65 • 10 years ago

Comment on attachment 8524481 [details] [review]
pull request for v2.0

Thank you for pointing it out. I've updated the code, please see if there's anything missing. Thank you!

	Dale Harvey (:daleharvey)
	Comment 66 • 10 years ago

Comment on attachment 8524481 [details] [review]
pull request for v2.0

Ok this patch looks good, thanks for the update, havent been able to reproduce the same issue with it, I will ask approval at the same time.


NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Existing bug
[User impact] if declined: Possible to get browser into a state that needs restarted
[Testing completed]: Manual testing fairly thorough, there are no automated tests in place for this functionality on 2.0
[Risk to taking this patch] (and alternatives if risky): Its an old area of code with little automated testing so not without risk, however the code is fairly clearly less risky than what exists.
[String changes made]: N/A

	Gregor Wagner [:gwagner]
	Comment 68 • 10 years ago

Who has to approve this?

	bhavana bajaj [:bajaj]
	Comment 69 • 10 years ago

(In reply to Gregor Wagner [:gwagner] from comment #68)
> Who has to approve this?

I was waiting for master/2.1 landing and realized those are not-impacted after reading the history. If helps if folks can set the status flags to reflect that.

	bhavana bajaj [:bajaj]
	Comment 70 • 10 years ago

Comment on attachment 8524481 [details] [review]
pull request for v2.0

Approving but given this is branch specific, NI brain huang to make sure his team verifies this post landing.

	Brian Huang [:brianhuang][:brhuang]
	Comment 71 • 10 years ago

William, please help on this. Thx.

	Ryan VanderMeulen [:RyanVM]
	Comment 72 • 10 years ago

According to Github, this was merged 2 days ago (without approval?!?).

v2.0: https://github.com/mozilla-b2g/gaia/commit/a376c95c3c3807dd5bbd26f6c2d41db10e20e672

	yifan [:yifan][:yliao] Assignee
	Comment 73 • 10 years ago

Sorry, that was my mistake. It got reversed instantly.

	William Hsu [:whsu]
	Comment 74 • 10 years ago

I can reproduce this bug on "20141126000203"
But, I cannot reproduce this bug on my local build (Merging yifan's patch).
Attach the demo video:
 - Before: Reproduce_Bug1059650.mp4
 - After: Verify_Bug1059650.mp4

* Build information:
 - Gaia-Rev        4aa775e0b6b1bb4dbf4a24ec14d7b7727930bb9c
 - Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/45112935086f
 - Build-ID        20141126000203
 - Version         32.0
 - Device-Name     flame
 - FW-Release      4.4.2
 - FW-Incremental  eng.cltbld.20141126.032754
 - FW-Date         Wed Nov 26 03:28:05 EST 2014


--- -- - --- -- - --- -- - --- -- - --- -- -
commit 4aa775e0b6b1bb4dbf4a24ec14d7b7727930bb9c
Author: William Hsu <whsu@mozilla.com>
Date:   Thu Nov 27 16:55:28 2014 +0800

    # 1059650 - [Browser]Back button stops working
    1623     tap: function tabSwipe_tap(e) {
    1624       if (this.browser.inTransition) {
    1625         return;
    1626       }
    1627 
    1628     this.isCloseButton = e.target.nodeName === 'BUTTON';
    1629     this.tab = this.isCloseButton ? e.target.parentNode : e.target;
    1630     this.id = this.tab.getAttribute('data-id');
    1631     this.containerWidth = this.tab.parentNode.clientWidth;
--- -- - --- -- - --- -- - --- -- - --- -- -

	William Hsu [:whsu]
	Comment 77 • 10 years ago

Please merge this patch if you also think this patch is low-risk.
Thanks.

	vsireesha246 Reporter
	Comment 78 • 10 years ago

I can reproduce this issue by doing STR mentioned in video Comment#19 in V2.0 branch.
Is this patch depends on any other patch?

	yifan [:yifan][:yliao] Assignee
	Comment 79 • 10 years ago

The patch hasn't been merged into v2.0.

	vsireesha246 Reporter
	Comment 80 • 10 years ago

HI Yliao,

I taken the patch of https://github.com/mozilla-b2g/gaia/pull/26228/files in V2.0 and tested it.
The issue is reproduciable.

Thanks..
Sireesha

	yifan [:yifan][:yliao] Assignee
	Comment 81 • 10 years ago

Thanks, will try to reproduce it shortly.

	William Hsu [:whsu]
	Comment 82 • 10 years ago

(In reply to vsireesha246 from comment #78)
> I can reproduce this issue by doing STR mentioned in video Comment#19 in
> V2.0 branch.
> Is this patch depends on any other patch?

Perhaps, you can manually check the file to see if you pull the correct file. Thanks.
Gaia_Repo > /apps/browser/js/browser.js

...
1623     tap: function tabSwipe_tap(e) {
1624       if (this.browser.inTransition) {
1625         return;
1626       }
1627 
1628     this.isCloseButton = e.target.nodeName === 'BUTTON';
1629     this.tab = this.isCloseButton ? e.target.parentNode : e.target;
1630     this.id = this.tab.getAttribute('data-id');
1631     this.containerWidth = this.tab.parentNode.clientWidth;
1632 
...

	vsireesha246 Reporter
	Comment 83 • 10 years ago

I checked again this issue by taking Latest V2.0 Gaia and applied the patch https://github.com/mozilla-b2g/gaia/pull/26228/files an tested again.

I can reproduce this issue and i can see below error 
11-28 14:27:20.021: E/GeckoConsole(3786): [JavaScript Error: "TypeError: t1 is null" {file: "app://browser.gaiamobile.org/shared/js/gesture_detector.js" line: 238}]

Note:Please while doing STR,double click '+' and back button.

	yifan [:yifan][:yliao] Assignee
	Comment 84 • 10 years ago

Confirmed that it could still be reproduced following the steps in Comment#19. I'd see what's missing in gesture_detector.js.

	Ben Francis [:benfrancis]
	Comment 85 • 10 years ago

Comment on attachment 8524481 [details] [review]
pull request for v2.0

Yifan, note that the mousedown event handler sets this.tab which is also used by the swipe and pan event handlers. If the mousedown event doesn't fire (which seems to happen intermittently) then these event handlers are likely to behave unpredictably as well.

	Gregor Wagner [:gwagner]
	Comment 86 • 10 years ago

Any updates here?

	Keven Kuo [:kkuo]
	Comment 87 • 10 years ago

NI YiFan

--
Keven

	yifan [:yifan][:yliao] Assignee
	Comment 88 • 10 years ago

I figured that there are 2 issues in this bug.

One is the 'this.currentTab is null' problem. Could be fixed by modifying tabsSwipeMngr as Ben suggested.

Another is the major cause that contribute to the STR in comment 19. When the new tab button is clicked, it'll eventually called stopDetecting after 200ms. But the subsequent tapping on the '<' button in this 200ms interval will cause a state problem in the GestureDetector because stopDetecting will be fired before the '<' action finished.

Working on a patch on both issues.

	yifan [:yifan][:yliao] Assignee
	Comment 89 • 10 years ago

* Change 'mousedown' event to 'touchstart' event for tabsSwipeMngr to fix 'this.currentTab is null' problem.

* Stop screenSwipeMngr gestureDetector right after new tab button clicked to fix the problem in comment 19.

	yifan [:yifan][:yliao] Assignee
	Comment 90 • 10 years ago

Thank you Ben for pointing out the missing part in comment 85. Instead of moving the code from 'mousedown' event to 'tap' event, I changed the 'mousedown' event to 'touchstart' to ensure the 'this.currentTab' be set correctly.

	Ben Francis [:benfrancis]
	Comment 91 • 10 years ago

Comment on attachment 8534194 [details] [review]
pull request for v2.0

Thanks Yifan, looks good to me. vsireesha, can you confirm?

	Dale Harvey (:daleharvey)
	Comment 92 • 10 years ago

Comment on attachment 8534194 [details] [review]
pull request for v2.0

Looks good to me as well, cheers

	vsireesha246 Reporter
	Comment 93 • 10 years ago

Hi Ben & Yliao,

I checked again this issue by taking Latest V2.0 Gaia and applied the patch https://bugzilla.mozilla.org/attachment.cgi?id=8534194&action=edit and tested again.

I can reproduce this issue and i can see below error 
11-28 14:27:20.021: E/GeckoConsole(3786): [JavaScript Error: "TypeError: t1 is null" {file: "app://browser.gaiamobile.org/shared/js/gesture_detector.js" line: 238}]

Note:I can also reproduce this issue with the patch of Bug-1011669 as well.

	vsireesha246 Reporter
	Comment 94 • 10 years ago

I can able to reproduce this issue in V1.1 as well.

	yifan [:yifan][:yliao] Assignee
	Comment 95 • 10 years ago

Thank you. Did you use the steps in comment 20 to reproduce the bug? Or you have another way to do it?

	vsireesha246 Reporter
	Comment 96 • 10 years ago

I can able to reproduce this issue while doing the STR in comment 20 and along with some normal operations like tab close and browse some pages.But this time the frequency of issue occurrence is less compared to previous after taking the patch of Bug-1011669.

	yifan [:yifan][:yliao] Assignee
	Comment 97 • 10 years ago

I've tried comment 20 some 50 times but haven't been able to reproduce. After the patch applied it's unlikely that this error could happen... Also I haven't seen the 't1 is null' error on my flame since the beginning. 

It could be some erroneous path that I didn't notice. What would you say the error rate of it in general? Are you able to specify what makes it easier to reproduce?

	Gregor Wagner [:gwagner]
	Comment 98 • 10 years ago

We are running out of time here. Lets land this patch and file a follow up if there is another STR.

	yifan [:yifan][:yliao] Assignee
	Comment 99 • 10 years ago

Comment on attachment 8534194 [details] [review]
pull request for v2.0

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 1059650
[User impact] if declined: Possible to get browser into a state that needs restarted
[Testing completed]: Manual testing fairly thorough, there are no automated tests in place for this functionality on 2.0
[Risk to taking this patch] (and alternatives if risky): The original code would be malfunctioned occasionally, the modification is less risky than what exists.
[String changes made]: N/A

	yifan [:yifan][:yliao] Assignee
	Comment 100 • 9 years ago

Thank you! Merged into v2.0.

	yifan [:yifan][:yliao] Assignee
	Comment 101 • 9 years ago

Merged into v2.0m

https://github.com/mozilla-b2g/gaia/pull/27072

	Shine(Leave from Mozilla)
	Comment 102 • 9 years ago

This problem is verified NOT to happen by doing STR mentioned in Comment#20 in latest Woodduck2.0/Flame 2.0.
See attachments: Verify_video.MP4
Rate: 0/40

Woodduck 2.0 build:
Gaia-Rev        3954e471652245293b5387e7bc732e52834658e5
Gecko-Rev       bb95bcf5c2033a59b3261b93804151d1addb75eb
Build-ID        20150104050313
Version         32.0

Flame 2.0 build:
Gaia-Rev        3c9bb36d9ade1a0acd5e1d6cbb5057be7f5ad484
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/93f5c85a1d31
Build-ID        20150103000200
Version         32.0

	Shally Li（leave from Mozilla）
	Comment 103 • 9 years ago

Per Comment 102,set "Status" as "VERIFIED".