Closed Bug 878156 Opened 6 years ago Closed 6 years ago

Tabs not closed on back press if Fennec started from a cold state

Categories

(Firefox for Android :: General, defect)

24 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 25
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- verified
firefox25 --- verified
fennec 24+ ---

People

(Reporter: krudnitski, Assigned: bnicholson)

References

Details

Attachments

(2 files)

It's a lousy summary. Sorry about that. But here's the deal.

On my Nexus 4, using the Twitter webapp.

Firefox is not open.

Scrolling through Twitter feeds, I click on a link (to flickr or an article) and select to use Firefox to view the article. I view it, and I press 'back' and it brings me back to Twitter.

But Firefox is still open and the article is still present.

(If I already had Fx open and I clicked on another link in Twitter and select to use Firefox to read it, Firefox opens and I read the article. I click 'back' and the tab closes and I am back in Twitter.)

I would expect the article to close at the very least. I actually would expect the application to close down, since I didn't have it open to begin with. BUT at the very least, I would want the article closed down and maybe just show the about:home page if we can't close down the browser.
Assignee: nobody → bnicholson
tracking-fennec: ? → 24+
Paul, can you please find a regression-range for this, or at least post which builds are affected? We were able to reproduce this in mobile-dev triage.
Flags: needinfo?(paul.feher)
Keywords: reproducible
Status: NEW → ASSIGNED
The behaviour is the same for earlier versions of Firefox is also reproducible on Firefox for Android 20. I don't think that this is a regression
Flags: needinfo?(paul.feher)
Agreed -- I think we've always had this behavior (or at least for a very long time). At startup, we pretty completely ignore the intent that we're given, and we handle all URLs in the same way. An initial quick fix for this would be to just mark all startup URLs as external, but then that means clicking back after loading a bookmark shortcut from the launcher or from the phone's search will also close the tab when back is pressed.

I see at least 2 ways we can start with an external URL:
1) ACTION_VIEW: Searching/entering a URL from the phone's search function, or clicking a link from another application
2) ACTION_BOOKMARK: Clicking a bookmark shortcut on the home launcher

Ian, should both of these close the opened tab on back press, or only #1?

Assuming we want different behaviors, we'll need to look through the intent we received in onCreate() to handle it properly. One way to do this is to reuse the intent handling between onNewIntent() and onCreate() (which we should probably do regardless).
Flags: needinfo?(ibarlow)
Keywords: reproducible
Simple patch that shares intent handling logic between onCreate and onNewIntent. Removes the unused ACTION_LOAD action, and uses tab stubs for loading URLs instead of passing it off to Gecko.
(In reply to Brian Nicholson (:bnicholson) from comment #3)
> Agreed -- I think we've always had this behavior (or at least for a very
> long time). At startup, we pretty completely ignore the intent that we're
> given, and we handle all URLs in the same way. An initial quick fix for this
> would be to just mark all startup URLs as external, but then that means
> clicking back after loading a bookmark shortcut from the launcher or from
> the phone's search will also close the tab when back is pressed.
> 
> I see at least 2 ways we can start with an external URL:
> 1) ACTION_VIEW: Searching/entering a URL from the phone's search function,
> or clicking a link from another application
> 2) ACTION_BOOKMARK: Clicking a bookmark shortcut on the home launcher
> 
> Ian, should both of these close the opened tab on back press, or only #1?
> 

Probably both. I can't see any benefit to the back button behaving differently for these two cases.
Flags: needinfo?(ibarlow)
To clarify comment 3: when the back button is clicked, the active tab is closed only if it is flagged as external. Currently, when Fennec is started from a cold state, the new tab is never marked as external, so the new tab will never be closed on back press.

This bug is still reproducible even if there are multiple tabs open. CC'ing Cwiiis since this is likely the issue he's been running into.
Summary: Fx keeps an article open if no other tabs are open → Tabs not closed on back press if Fennec started from a cold state
Given comment 5, we can fix this simply by marking all startup tabs as external. It would still be good to consolidate the onNewIntent() logic with the onCreate() logic to avoid different paths for the same actions, but that's not necessary for this fix.
Attachment #783204 - Flags: review?(mark.finkle)
Attachment #783204 - Flags: review?(mark.finkle) → review+
Comment on attachment 783204 [details] [diff] [review]
Mark all startup tabs as external

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 797075
User impact if declined: If a link from some app (such as twitter) is used to launch Fennec, pressing back will not close the tab if Fennec wasn't already running.
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): very low risk
String or IDL/UUID changes made by this patch: none

An old bug that's been around for awhile, but would still nice to be fixed as it's been reported by several users.
Attachment #783204 - Flags: approval-mozilla-aurora?
Blocks: 797075
https://hg.mozilla.org/mozilla-central/rev/392d600662e0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Comment on attachment 783204 [details] [diff] [review]
Mark all startup tabs as external

although an old bug,approving as the patch is low risk.

Will approach backout of regressions appear and thers is no low risk alternative.

Also adding verifyme for QA help with verification.
Attachment #783204 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.