578711 - Opening two copies of "Tabs from my Other Computers" closes Fennec

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 0. Configure Fennec with a Weave account
 1. Drag Fennec to the right, and click the "tabs from my other computers" icon in lower-left corner.
 2. After that page loads, repeat step 1.

ACTUAL RESULTS: Crash. No crash reporter dialog shows up.

VERSION INFO:
I'm using a Fennec 2.0 nightly, up-to-date from the repos, with "Gecko/20100713" in the UA string.
This is on a Nokia n900.

Comment 1

[Martijn Wargers (dead)]

Confirmed. I got this crash by clicking 2 times on the "tabs from my other computers" button on the about:firstrun page.

Comment 2

[Daniel Holbert [:dholbert]]

NOTE: If I close the first instance of "Tabs from my other computers" before opening the second one, I don't get a crash.

So, I think the basic problem just happens whenever you have two copies of this page open at once.

Comment 3

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

Attachment: patch

This patch adds a method to BrowserUI to select an existing tab before opening a new one. Better UX and it "fixes" the bug.

I will try to find the real reason the window closes too.

Comment 4

[Daniel Holbert [:dholbert]]

Note that even with the attached patch, you can still trigger this bug by manually going to "about:sync-tabs" in two different tabs.  (you probably know that already though)

Comment 5

[Martijn Wargers (dead)]

Why doesn't the crash reporter come up with this particular crash?

+      if (tabs[i].browser.currentURI.spec == "about:sync-tabs") {
Shouldn't "about:sync-tabs" be aURI?

So that patch selects an existing tab, but shouldn't it also reload that existing tab, since content could have changed?

Comment 6

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

(In reply to comment #5)
> Why doesn't the crash reporter come up with this particular crash?
> 
> +      if (tabs[i].browser.currentURI.spec == "about:sync-tabs") {
> Shouldn't "about:sync-tabs" be aURI?

Yes!

> So that patch selects an existing tab, but shouldn't it also reload that
> existing tab, since content could have changed?

Not really. This method shouldn't be called for all types of pages, and it works like the Firefox "open existing tab" feature - which does not reload the page.

New patch coming up

I am more worried about comment 4

Comment 7

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

Attachment: patch

Comment 8

[Martijn Wargers (dead)]

From IRC:
[21:00]	<mfinkle>	dholbert, mw22: there is no crash!
[21:00]	<mfinkle>	the window just decides to close

So I'm updating the summary to reflect that.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
>--- a/chrome/content/browser-ui.js
>+++ b/chrome/content/browser-ui.js

>-  _openTab: function _openTab(url) {
>-    setTimeout(function() BrowserUI.newTab(url), 0);
>-  },
>-

I'm just worried  about this setTimeout wrap we remove.

Comment 10

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

The setTimeout was from the days when sync was an add-on and needed to worry about the awesomebar screen appearing. Doesn't affect anything now (it seems)

pushed:
http://hg.mozilla.org/mobile-browser/rev/36723271befb

Comment 11

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

Filed bug 579346 for comment 4

Comment 12

[Cristian Nicolae (:xti)]

This feature has been changed on the latest Nightly build and it works as expected. I will close this issue as verified fixed.

--
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110920
Firefox/9.0a1 Fennec/9.0a1
Device: Samsung Galaxy S
OS: Android 2.2