The new window's size is modified if the last closed window had pinned tabs

RESOLVED FIXED in Firefox 50

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: simona.marcu, Assigned: mconley)

Tracking

({regression})

48 Branch
Firefox 50
All
macOS
Points:
---

Firefox Tracking Flags

(e10s+, firefox48 wontfix, firefox49 fix-optional, firefox50 fixed)

Details

(Whiteboard: tpi:+)

Attachments

(2 attachments)

Posted video Recording #1.mp4
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160509040557

[Affected versions]:
- Firefox Nightly 49.0a1
- Firefox Developer Edition 48.0a2

[Affected platforms]:
- Mac OS X

[Steps to reproduce]:
1. Launch Firefox with a new profile
2. Pin 2 tabs 
3. Close Firefox by clicking on the "x" button located on the upper left side of the window
4. In the Dock bar, click on the Firefox icon to open a new window


[Expected result]:
The size of the new window should be the same as the size of the last closed window.

[Actual result]:
The new window is shrunk. Please see the screencast for more details.

[Regression range]:
- This is a regression, I will come back with the regression window as soon as possible.

[Additional notes]:
- Not reproducible on Firefox 46.0.1 RC and on Firefox 47 beta 3.
Hey Simona, did you find the regression range here?
Flags: needinfo?(simona.marcu)
Priority: -- → P1
Whiteboard: tpi:+
Sorry for the delay, somehow it fell off my radar.

Last good revision: ae2ec0e9bc9ffa8417ab2e6f5672f955eba60e1c
First bad revision: eff4131a3e4caf33761bd52cb9a5f2dd5601c4f1

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ae2ec0e9bc9ffa8417ab2e6f5672f955eba60e1c&tochange=eff4131a3e4caf33761bd52cb9a5f2dd5601c4f1
Flags: needinfo?(simona.marcu)
tracking-e10s: --- → ?
(I think this is more likely to be a front-end bug, but who knows!)
Component: Widget: Cocoa → General
Product: Core → Firefox
Mike, feels like an issue with some of the code in bug 1209689 and bug 1220929?

(can't needinfo him yet, will do it when he's back)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #4)
> Mike, feels like an issue with some of the code in bug 1209689 and bug
> 1220929?
> 
> (can't needinfo him yet, will do it when he's back)
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mconley)
I've bisected this down to a single changeset:

changeset:   312446:243353ae9c08
user:        Mike Conley <mconley@mozilla.com>
date:        Wed Oct 28 15:25:03 2015 -0400
summary:     Bug 1209689 - Tabs that haven't yet been restored should not crash. r=Mossop

It looks like the forcing of the tab to be restored to be non-remote is at fault here. It’s possible that a remoteness flip is occurring for the pinned tabs (which load their content right away) at a super inconvenient time which prevents the window from sizing correctly. Hard to say.

An easy solution, which seems to fix the issue, is to not force those tabs to be non-remote if the tab is going to be pinned, since it should load its content right away.

I'm not sure if people really want me to dig into the why of this, but I've got a patch coming up which seems to fix the issue (just waiting on my build to finish on tip of central to be sure).
Assignee: nobody → mconley
Flags: needinfo?(mconley)
App tabs load immediately, and so there's no point in causing the remoteness
flipping machinery to kick off by making the pinned tabs non-remote by default.

Review commit: https://reviewboard.mozilla.org/r/61330/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61330/
Attachment #8766447 - Flags: review?(mdeboer)
At least with my local testing, this appears to fix the issue. Again, I'm not 100% certain _why_ this causes the problem to present, but this patch at the very least fixes a real performance problem, where we're kicking off the remoteness flipping machinery needlessly for pinned tabs.

My sorta hand-wavey hypothesis for why making those pinned tabs non-remote by default causes this bug is that when the window opens and the tabs start loading, we do a remoteness flip at the time when the window attempts to do an intrinsic size, and I guess somehow the removal and re-addition of the browsers for the flip somehow causes that size calculation to not work. Again, pretty hand-wavey, but I'm not sure it's worth a full-scale investigation, tbh, unless somebody wants to push back.
Comment on attachment 8766447 [details]
Bug 1271607 - Pinned tabs should not restore as non-remote.

https://reviewboard.mozilla.org/r/61330/#review58442

It's not going to be me who'll push back ;-)

::: browser/components/sessionstore/SessionStore.jsm:2939
(Diff revision 1)
>        let reuseExisting = t < openTabCount &&
>                            (tabbrowser.tabs[t].getAttribute("usercontextid") == (userContextId || ""));
> +      // If the tab is pinned, then we'll be loading it right away, and
> +      // there's no need to cause a remoteness flip by loading it initially
> +      // non-remote.
> +      let forceNotRemote = !winData.tabs[t].pinned;

This positive vs. negative variable naming collision is making my brain melt!
Attachment #8766447 - Flags: review?(mdeboer) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6cce38101acc
Pinned tabs should not restore as non-remote. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/6cce38101acc
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Hi Mike, do you want to uplift this for 48/49 if this patch is not too risky.
Flags: needinfo?(mconley)
(In reply to Gerry Chang [:gchang] from comment #12)
> Hi Mike, do you want to uplift this for 48/49 if this patch is not too risky.

I'm not sure this warrants an uplift. This doesn't strike me as a common bug, especially if the user has ever resized the window, I believe we sidestep the issue. I believe it's only for new profiles that the STR cause the bug.

I think the patch is low risk, so I could go either way to be honest. Do you feel like it warrants uplift?
Flags: needinfo?(mconley) → needinfo?(gchang)
I think this is ok to wontfix for 48/49. At most we could bring this up to 49 aurora but it does seem like an uncommon use.
QA Whiteboard: [good first verify]

(In reply to Simona B [:simonab ] from comment #0)
> Created attachment 8750748 [details]
> Recording #1.mp4
> 
> Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:49.0) Gecko/20100101
> Firefox/49.0
> Build ID: 20160509040557
> 
> [Affected versions]:
> - Firefox Nightly 49.0a1
> - Firefox Developer Edition 48.0a2
> 
> [Affected platforms]:
> - Mac OS X
> 
> [Steps to reproduce]:
> 1. Launch Firefox with a new profile
> 2. Pin 2 tabs 
> 3. Close Firefox by clicking on the "x" button located on the upper left
> side of the window
> 4. In the Dock bar, click on the Firefox icon to open a new window
> 
> 
> [Expected result]:
> The size of the new window should be the same as the size of the last closed
> window.
> 
> [Actual result]:
> The new window is shrunk. Please see the screencast for more details.
> 
> [Regression range]:
> - This is a regression, I will come back with the regression window as soon
> as possible.
> 
> [Additional notes]:
> - Not reproducible on Firefox 46.0.1 RC and on Firefox 47 beta 3.



Guys i cannt reproduce this bug and im not understanding clearly.
(In reply to Simona B [:simonab ] from comment #0)
> Created attachment 8750748 [details]
> Recording #1.mp4
> 
> Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:49.0) Gecko/20100101
> Firefox/49.0
> Build ID: 20160509040557
> 
> [Affected versions]:
> - Firefox Nightly 49.0a1
> - Firefox Developer Edition 48.0a2
> 
> [Affected platforms]:
> - Mac OS X
> 
> [Steps to reproduce]:
> 1. Launch Firefox with a new profile
> 2. Pin 2 tabs 
> 3. Close Firefox by clicking on the "x" button located on the upper left
> side of the window
> 4. In the Dock bar, click on the Firefox icon to open a new window
> 
> 
> [Expected result]:
> The size of the new window should be the same as the size of the last closed
> window.
> 
> [Actual result]:
> The new window is shrunk. Please see the screencast for more details.
> 
> [Regression range]:
> - This is a regression, I will come back with the regression window as soon
> as possible.
> 
> [Additional notes]:
> - Not reproducible on Firefox 46.0.1 RC and on Firefox 47 beta 3.



Guys i cant reproduce this bug and im not understanding this Bug. would someone help explain more please?
You need to log in before you can comment on or make changes to this bug.