583770 - Navigating any of the system pages (about:) opens an extra blank tab in background

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

Attachment: screenshot

Steps: 
1. Open a new tab
2. Navigate to about:rights or any other system page
3. Slide the screen from left to right to bring up tab manager
4. Inspect the number of tabs now

================================================================================

Expected:
 
System page should load as expected. 
You should not have any extra tabs besides the tabs you are browsing on. 

================================================================================

Actual:

There is an extra-blank tab which is opened as soon as the system page loads in
content area.

Comment 1

[Aakash Desai [:aakashd]]

Confirmed on build:
Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b2pre) Gecko/20100802 Namoroka/4.0b3pre Fennec/2.0a1pre

Comment 2

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

From the steps to reproduce in comment 0;
1. Open a new tab
2. Type in "about:config" (or any about: page) and press enter.

Step 1 creates a new "remote" tab, since "about:blank" is considered "remote".
Step 2 tries to load a "local" page, which means we need to open a new "local"
tab since the current new tab was created as a "remote" tab.

We need to add some code to BrowserUI.goToURI to remove the new, empty "about:blank" tab in this situation.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: patch

We still call stop() twice when creating a new browser here.  Fixing this would require propagating the loadFlags all the way through addTab to tab._createBrowser, which seemed messier than it was worth.

Comment 4

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

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

>diff -r 663a2f6aa05a chrome/content/browser-ui.js

>+    Browser.loadURI(aURI, {flags: Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP});

>+    Browser.loadURI(submission.uri.spec, {postData: submission.postData});

Add spaces around the { }

>diff -r 663a2f6aa05a chrome/content/browser.js

>+  loadURI: function loadURI(aURI, aParams) {

>+    if (hasLocal != useLocal) {
>+      if (currentURI == "about:blank" && !browser.canGoBack && !browser.canGoForward)
>+        this.closeTab(this.selectedTab);
>+      let tab = Browser.addTab(aURI, true, this.selectedTab);

Do we always want "this.selectedTab" to be the owner? What if the user opened a new tab manually, which you check for above?

r+, but we should answer the owner question before landing

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

(In reply to comment #4)
> Add spaces around the { }

Done.

> Do we always want "this.selectedTab" to be the owner? What if the user opened a
> new tab manually, which you check for above?
> 
> r+, but we should answer the owner question before landing

Good point.  I updated the patch to not set an owner after closing the selected tab.

Comment 6

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mobile-browser/rev/dc364482bdfc

Comment 7

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b4pre) Gecko/20100811 Namoroka/4.0b4pre Fennec/2.0a1pre

and

Mozilla/5.0 (Android; U; Linux armv71; en-US; rv:2.0b4pre) Gecko/20100811 Namoroka/4.0b4pre Fennec/2.0a1pre