New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 871977 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Webextensions' isWindowClosing from chrome.tabs.onRemoved info is always true

Reported by seb.bl...@gmail.com, Aug 7

Issue description

UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/68.0.3440.75 Chrome/68.0.3440.75 Safari/537.36

Steps to reproduce the problem:
1. Create a webextension with following code: chrome.tabs.onRemoved.addListener((id, info) => { console.log(id, info); });
2. close the last tab
3. reopen the browser and close the window

What is the expected behavior?
When closing the last tab, info.isWindowClosing should be false, because the tab is not closed due to the window being closed (this is the behaviour pre Chromium 68). It should only be true if the window is closed explicitly. 

What went wrong?
Since Chromium 68, info.isWindowClosing is true when I close the last tab and when I close the window which makes it useless.

Did this work before? Yes 67

Chrome version: 68.0.3440.75  Channel: n/a
OS Version: 
Flash Version: 

For the code where this matters look at https://chrome.google.com/webstore/detail/do-not-close-browser-with/iiolkehjeklhkdphaakkceadenbcdahj
 
Labels: Needs-Triage-M68 Needs-Bisect
Cc: viswa.karala@chromium.org
Labels: -Pri-2 -Needs-Bisect ReleaseBlock-Stable Triaged-ET Target-68 Target-69 Target-70 RegressedIn-68 FoundIn-70 M-68 FoundIn-69 FoundIn-68 hasbisect OS-Windows Pri-1
Owner: erikc...@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce the issue on reported version 68.0.3440.75, 68.0.3440.84 and on latest chrome# 70.0.3515.0 using Ubuntu 14.04 and Windows-10, hence providing Bisect Info
Note: Issue is not applicable to Mac
Bisect Info:
================
Good build: 68.0.3430.0
Bad build: 68.0.3431.0

On running per-revision bisect, got error: "Exception: Error running the gsutil command: ServiceException: 401 Anonymous caller does not have storage.objects.get access to chrome-test-builds/official-by-commit/Win x64 Builder" and below is the change-log from chromium bisect

You are probably looking for a change made after 558524 (known good), but no later than 558530 (first known bad).
https://chromium.googlesource.com/chromium/src/+log/dd09cdf9373c2d180da6a82d2eda2d49dafb9508..f64c33d60de88b6b0d1a3d65860a710ab37dafac
Change-Id: I2fc54749e495ab9c925bdba89ef857c4ea3537d9
Reviewed-on: https://chromium-review.googlesource.com/1055732

@erikchen: Please confirm the issue and help in re-assigning if it is not related to your change.
Adding ReleaseBlock-Stable as it is seems a recent break, feel free to remove it if not applicable.

Thanks!
Cc: abdulsyed@chromium.org manoranj...@chromium.org
Labels: M-69
This feature was added here: 
https://bugs.chromium.org/p/chromium/issues/detail?id=56592

"""
Because closing a window first triggers onTabClosed events for each tab, there is no way to tell whether a tab is closing because the user explicitly closed that tab, or because the user closed the whole window.

An example of how this would be useful is my extension SimpleWindowSaver. When a user closes a tab in a saved window, I want to remove that tab from the saved state. However, when the user closes the window, I want to save all the state so they can restore it later.
"""

This leaves open the question -- what is the appropriate behavior when a user closes the last tab in a window, which will also close the Window. This isn't particularly well defined.

Right now, we set closing_all_ = true in two places:
  1) CloseAllTabs() -- this gets called when closing the Window
  2) DetachWebContentsImpl, when detaching the last tab. 

(2) violates the public comment on closing_all():
"""
145   // Returns true if the tabstrip is currently closing all open tabs (via a                                                                                                                                                                
146   // call to CloseAllTabs)
"""

Looking at all callsites for closing_all(), they appear to want the semantics described in the comment. As such, removing (2) should fix this bug, and potential fix some other similar issues.
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25919fcea95287f1be0ac0f686cbfc118faa1aca

commit 25919fcea95287f1be0ac0f686cbfc118faa1aca
Author: erikchen <erikchen@chromium.org>
Date: Fri Aug 10 13:17:41 2018

Change implementation of TabStripModel::closing_all().

The semantics of TabStripModel::closing_all() [both in the header comments, and
in usage by callsites] is that it should only be set to |true| when
CloseAllTabs() has been called.

However, it was also being set to |true| in
TabStripModel::DetachWebContentsImpl. This was incorrect. It appears to have
been present in the initial commit of chrome/ to the Chromium repository:
09911bf300f1a419907a9412154760efd0b7abc3.

Bug:  871977 
Change-Id: Ied0dcb79611f1c2b144657dec897551d3734a38a
Reviewed-on: https://chromium-review.googlesource.com/1169622
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582130}
[modify] https://crrev.com/25919fcea95287f1be0ac0f686cbfc118faa1aca/chrome/browser/ui/tabs/tab_strip_model.cc

Labels: TE-Verified-M70 TE-Verified-70.0.3521.0
Able to reproduce the issue on chrome reported version 68.0.3440.75
Verified the fix on Windows-10 & Ubuntu 14.04 on Chrome version #70.0.3521.0 as per the comment#0
Attaching screen cast for reference.
Observed "Unable to close the window through tab close button when only one tab is present"
Hence, the fix is working as expected.
Adding the verified label.

Thanks!
871977.ogv
2.7 MB View Download
Status: Fixed (was: Assigned)
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-69; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-69 label, otherwise remove Merge-TBD label. Thanks.
This is regressed in M68, so lets target fix for M70. Pls let me know ASAP if there is any concern here. 
That sounds good.
Labels: -M-69 -M-68 -Target-68 -Target-69 M-70
Thank you  erikchen@. 

Removing M68/M69 milestones.
Labels: -Merge-TBD

Sign in to add a comment