752688 - Tab count not visible

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jaclyn Fu]

Attachment: screenshot

Web page or screen you were on when you saw the issue: all

Steps to reproduce:
1. Opened Aurora
2. Tap on tab to open new tab
3. Have multiple tabs opened

What you expected: able to see tab count

Comment 1

[Michelle Luna]

I see this on my galaxy nexus with ICS today also, see attached additional screenshot.

Comment 2

[Michelle Luna]

Attachment: screenshot of empty tab

Comment 3

[:Margaret Leibovic]

If either of you see this again, could you attach logs?

Comment 4

[Jaclyn Fu]

Quick instructions on how to find logs? :)

Comment 5

[:Margaret Leibovic]

(In reply to Jaclyn Fu from comment #4)
> Quick instructions on how to find logs? :)

https://wiki.mozilla.org/Mobile/Testdrivers_Program/How_To/File_a_Bug#If_possible.2C_get_a_Log :)

Comment 6

[Michelle Luna]

Attachment: log of tab increment issue (sometime around 11:09/4:09)

Comment 7

[:Margaret Leibovic]

Looking at the logs, nothing jumps out at me as causing the problem. Unfortunately, we don't currently have much logging around updating the tab count (maybe I should write a patch to add some).

Can anyone figure out steps to reproduce this? Michelle or Jaclyn, do you remember what you were doing when this happened?

Comment 8

[Aaron Train [:aaronmt]]

Although not the steps reported; another way to get into this state is via changing the system language while the Fennec activity is currently running (i.e, Settings -> Language & input -> Language -> Français), and switch back to Fennec.

Comment 9

[:Margaret Leibovic]

(In reply to Aaron Train [:aaronmt] from comment #8)
> Although not the steps reported; another way to get into this state is via
> changing the system language while the Fennec activity is currently running
> (i.e, Settings -> Language & input -> Language -> Français), and switch back
> to Fennec.

Thanks for that - I'll look into what happens there. Before seeing this comment, I wrote this...

So, doing some code investigation, this could only happen if we both hide the number (mTabsCount.setVisibility(View.GONE)) *and* we make the tab icon image invisibile (mTabs.setImageLevel(0)).

We call mTabs.setImageLevel(0) during initialization, and mTabsCount is hidden to begin with, so I suppose we could get into this state if we somehow never call updateTabCount or updateTabCountAndAnimate, but that seems unlikely.

The calls to mTabs.setImageLevel() in updateTabCountAndAnimate will never call it with 0, so those can't be guilty.

The only other explanation is that we're calling updateTabCount(0), since that would call mTabs.setImageLevel(0) and hide mTabsCount. There are two places this could happen:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserToolbar.java#377
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#298

The first will only happen on devices that use ActionBar, and since this happened on Jaclyn's gingerbread device, that call in Tabs is all that's left.

Jaclyn and Michelle, do you remember if this happened after coming back to the app after it had been killed in the background?

Brian, is there any way that Tabs.getCount() could return 0 at the end of session restore?

Comment 10

[:Margaret Leibovic]

(Brian, see above comment)

Comment 11

[Jaclyn Fu]

I think it was after I came back to the app after it was killed

Comment 12

[Brian Nicholson (:bnicholson)]

(In reply to Margaret Leibovic [:margaret] from comment #9)
> Brian, is there any way that Tabs.getCount() could return 0 at the end of
> session restore?

In the screenshot, it looks like the page has loaded and there's a title - basically, everything seems to be working (excluding the tab counter). I don't think it's possible for a page to be shown if there are no tabs.

Maybe Session:RestoreEnd isn't getting called somehow? That would be one explanation why everything else would be working. Restore failures should be caught - we have a try/catch that sends a "fail" message using notifyObservers(). But that function first calls _clearCache() before notifying observers (http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#920), so that might somehow be failing.

Comment 13

[Michelle Luna]

Same here, the counter disappeared after restarting from a crash.

Comment 14

[Brian Nicholson (:bnicholson)]

From Michelle's log:

05-07 22:59:42.591: INFO/GeckoTabs(15724): Got message: Session:RestoreBegin
05-07 22:59:42.599: INFO/GeckoApp(15724): Got message: Gecko:Ready
05-07 22:59:42.599: DEBUG/GeckoLayerClient(15724): Screen-size changed to (720,1184)
05-07 22:59:42.599: DEBUG/GeckoLayerClient(15724): Window-size changed to (720,1038)
05-07 22:59:42.607: INFO/GeckoLayerClient(15724): Showing checks: true
05-07 22:59:42.615: INFO/Gecko(15724): Detected osrelease `3.0.8-gda6252b'
05-07 22:59:42.615: INFO/Gecko(15724): JITs are not broken
05-07 22:59:42.654: INFO/GeckoTabs(15724): Got message: Tab:Added
05-07 22:59:42.654: INFO/GeckoTabs(15724): Received message from Gecko: 42983835 - Tab:Added
05-07 22:59:42.662: INFO/GeckoTabs(15724): Added a tab with id: 1
05-07 22:59:42.693: INFO/GeckoTabs(15724): Got message: Tab:Added
05-07 22:59:42.693: INFO/GeckoTabs(15724): Received message from Gecko: 42983867 - Tab:Added
05-07 22:59:42.693: INFO/GeckoTabs(15724): Added a tab with id: 2
05-07 22:59:42.716: INFO/GeckoTabs(15724): Got message: Tab:Added
05-07 22:59:42.716: INFO/GeckoTabs(15724): Received message from Gecko: 42983894 - Tab:Added
05-07 22:59:42.716: INFO/GeckoTabs(15724): Added a tab with id: 3
05-07 22:59:42.740: INFO/GeckoTabs(15724): Got message: Tab:Added
05-07 22:59:42.740: INFO/GeckoTabs(15724): Received message from Gecko: 42983919 - Tab:Added
05-07 22:59:42.740: INFO/GeckoTabs(15724): Added a tab with id: 4
05-07 22:59:42.748: INFO/GeckoTabs(15724): Got message: SessionHistory:New
05-07 22:59:42.763: ERROR/GeckoConsole(15724): [JavaScript Error: "[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsISHEntry.adoptBFCacheEntry]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: jar:jar:file:///data/app/org.mozilla.fennec_aurora-1.apk!/omni.ja!/components/SessionStore.js :: _deserializeHistoryEntry :: line 747"  data: no]" {file: "jar:jar:file:///data/app/org.mozilla.fennec_aurora-1.apk!/omni.ja!/components/SessionStore.js" line: 747}]
05-07 22:59:42.771: INFO/GeckoPanZoomController(15724): Got message: Preferences:Data

Comment 15

[Brian Nicholson (:bnicholson)]

Note that there's no corresponding Session:RestoreEnd above.

_restoreHistory() is called in an async callback (NetUtil.asyncFetch()), so that explains why the error isn't caught.

Comment 16

[:Margaret Leibovic]

Attachment: patch to catch exception

Brian, thanks for figuring that out!

This patch just catches the exception, which will prevent SessionStore.js from dying. However, I'd like to figure out why adoptBFCacheEntry is failing, and if that will cause us other problems down the road.

Comment 17

[Justin Lebar (not reading bugmail)]

      let matchingEntry = aDocIdentMap[aEntry.docIdentifier];
      if (!matchingEntry) {
        matchingEntry = {shEntry: shEntry, childDocIdents: childDocIdents};
        aDocIdentMap[aEntry.docIdentifier] = matchingEntry;
      } else {
        shEntry.adoptBFCacheEntry(matchingEntry);   <------
        childDocIdents = matchingEntry.childDocIdents;
      }

Surely this should be adoptBFCacheEntry(matchingEntry.shEntry).  This looks right in nsSessionStore.js (the desktop version).

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 622819 [details] [diff] [review]
patch to catch exception

Let's fix the typo first. I think we might want to add a try/catch in the code that calls _deserializeHistory, to keep the entire system from ending up horked if something goes wrong.

Comment 19

[:Margaret Leibovic]

Attachment: patch

Thank you, Justin! I was comparing with the desktop version earlier, but somehow missed that.

I don't think we need the try/catch anymore, since this shouldn't be throwing anymore with this fix. I hope we don't have other silly mistakes like this :/

Comment 20

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e8b484241e9

Comment 21

[:Margaret Leibovic]

Comment on attachment 622900 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: session restore can die, resulting in blank tabs counter (and likely session restore bustage)
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): low-risk typo fix
String changes made by this patch: n/a

Comment 22

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/8e8b484241e9

Comment 23

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7bc24d1f43bf

Comment 24

[Cristian Nicolae (:xti)]

I cannot reproduce this bug on the latest Nightly and Aurora builds. Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-30)
Firefox 14.0a2 (2012-05-30)

Device: Galaxy Nexus
OS: Android 4.0.2