886576 - Unable to show the dynamic address bar on short Twitter pages

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

http://www.youtube.com/watch?v=Ie35sF4Op20

On a Nexus 4 I get into this state somewhat regularly. I don't have solid STR other than to use Twitter often.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Chris, any ideas here?

Comment 2

[Chris Lord [:cwiiis]]

So I guess there's some possibly situation where the page changes size or similar and the notification callback doesn't get triggered...

I don't know when this happens, but I think I can probably write some kind of work-around that makes sure the toolbar can always be made visible regardless.

Comment 3

[Chris Lord [:cwiiis]]

Attachment: Simple possible fix

Ugh, so I think this is just a silly comparison error... At least it's a small patch :) I have a patch for bug 892246 that may also help in certain situations of this.

Comment 4

[Chris Lord [:cwiiis]]

Comment on attachment 775667 [details] [diff] [review]
Simple possible fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Dynamic toolbar sometimes doesn't show when it should
User impact if declined: Can end up stuck in a situation where it's impossible to reveal the dynamic toolbar
Testing completed (on m-c, etc.): Tested locally
Risk to taking this patch (and alternatives if risky): Low risk
String or IDL/UUID changes made by this patch: None

Comment 5

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

1. i can reproduce the problem on my nexus 4 on FF23 beta on semi random tweets like this one:
https://twitter.com/_Reinette/status/356913948861267969

2. The apk at 
http://chrislord.net/files/mozilla/fix-inacccessible-toolbar.apk doesn't work on my Nexus 4 nor does it work on :kbrosnan's Nexus4

:cwiis Please compile a new APK that will actually work on a Nexus 7

Comment 6

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

oops i meant nexus 4 (NOT 7!)

Comment 7

[Chris Lord [:cwiiis]]

Pushed to m-c:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e26030f774d

Comment 8

[Chris Lord [:cwiiis]]

Don't know why my apk's aren't working, maybe just a failed upload - You'll be able to get apks from the build machine here once the build finishes (click on the 'B' next to 'Android 2.2 opt' and the link will be at the bottom-left):

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e7743ae9df87

Comment 9

[Chris Lord [:cwiiis]]

Here you go: http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-inbound-android/1373964790/fennec-25.0a1.en-US.android-arm.apk

Comment 10

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

> Here you go:
> http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-
> inbound-android/1373964790/fennec-25.0a1.en-US.android-arm.apk

I can't reproduce this problem in the above nightly APK. However the problem is extremely hard (it seems random) to reproduce in 23 so this doesn't mean the problem is fixed (my guess is that Chris Lord's patch helps and should be approved). I guess the next step is that assuming this patch passes review and
 :kbrosnan and :aaronmt don't find any bugs, to uploift to beta.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e26030f774d

Comment 12

[bhavana bajaj [:bajaj]]

Comment on attachment 775667 [details] [diff] [review]
Simple possible fix

:Kbrosnan can you please help with some adhoc testing here to help with verification on nightly or aurora given you were able to reproduce this ?

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 775667 [details] [diff] [review]
Simple possible fix

Risk is low enough we can take to Beta as well this week.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b487258a6450
https://hg.mozilla.org/releases/mozilla-beta/rev/5fdab6872b49

Comment 15

[Kevin Brosnan [Ex-Mozilla]]

Still reproduced this on the July 17th nightly - http://www.youtube.com/watch?v=LjxHMbkIIvo

Comment 16

[Chris Lord [:cwiiis]]

Attachment: Another simple possible fix

Ugh, so this is something we should commit regardless of whether it fixes the situation (but I hope it does) - we were locking the toolbar after starting the show animation instead of before. As the monitor isn't held over both operations,
there's the possibility the animation could be cancelled before the lock takes effect.

Comment 17

[Chris Lord [:cwiiis]]

Attachment: Mitigate dynamic toolbar not getting locked when it should

I'm not a fan of this patch, but it ought to mitigate situations where the dynamic toolbar is incorrectly not locked into position. I'd prefer we take the other simple fix and see if that sorts the issue before pushing this though, as I think it makes the code a bit uglier.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 778408 [details] [diff] [review]
Another simple possible fix

Review of attachment 778408 [details] [diff] [review]:
-----------------------------------------------------------------

Minusing as per IRC discussion. This doesn't make much of a difference because both those calls are nonblocking and will run regardless of whatever else happens. The animation can still be cancelled after the two lines of code and it will still be a problem.

Comment 19

[Chris Lord [:cwiiis]]

Attachment: Likely, simple, possible fix

kats helped me to notice that the animation-cancelling logic in LayerMarginsAnimator was wrong.

As well as reversing the call order in BrowserApp (which is probably not necessary, but could mitigate future threading issues), this also moves the cancelling inside the check to see if margins are pinned so that margin animations aren't cancelled by scrolling when margins are pinned.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 778411 [details] [diff] [review]
Mitigate dynamic toolbar not getting locked when it should

I also don't really like doing this. I'm going to drop the review flag for now but we can reconsider it later if the other patch doesn't do the job. I'm still leaning towards minusing this and trying to figure out the real fix for the issue.

Comment 21

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/c30555419404

Comment 22

[Chris Lord [:cwiiis]]

You seem to be rather adept at hitting this bug Kevin, if you could test out a build once this hits central (or at your nearest convenience) and report back, that'd be much appreciated!

I'll hold off on requesting for beta/aurora for now but let me know if you think different.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Resetting status flags to reflect this new landing - we can adjust once Kevin confirms inability to reproduce.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Kevin Brosnan [Ex-Mozilla]]

I have not been able to reproduce this while browsing on Twitter today and yesterday using Nightly. I would like to hold off on marking verified as it can be rather intermittent. Though I would recommend uplifting the patch for the next beta assuming there is still time.

Comment 26

[Chris Lord [:cwiiis]]

Comment on attachment 778479 [details] [diff] [review]
Likely, simple, possible fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Dynamic toolbar getting stuck in its hidden state
User impact if declined: Pages that shrink enough to show the toolbar permanently will occasionally hide it permanently instead
Testing completed (on m-c, etc.): Tested on m-c, no reports of regressions so far and one report that it may have fixed the issue
Risk to taking this patch (and alternatives if risky): Low risk
String or IDL/UUID changes made by this patch: None

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d45388fa2fe1
https://hg.mozilla.org/releases/mozilla-beta/rev/72b23bb6a190

Comment 28

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 25.0a1 (2013-07-28)
Device: LG Nexus 4
OS: Android 4.2.2

Comment 29

[Adrian Tamas (:AdrianT)]

Unable to reproduce the issue on Firefox Mobile 23 RC build 1 and Aurora 24.0a2 2013-07-31 on the Samsung Galaxy R (Android 2.3.4). Marking the issue as Verified Fix