1214064 - Transient window sizes from windows that the user resizes and closes now become permanent

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

STEPS TO REPRODUCE:

1) Have a browser window of the size you like.  Say 1000px by 1000px.
2) Open a new window.
3) Load, in that new window, some site that doesn't work well with your window
   size (e.g. styles the body to be 1200px wide).
4) Resize the new window to better fit that website.
5) Close the new window, focus your original 1000x1000 window.
6) Open a new window.

EXPECTED RESULTS: The new window is 1000px by 1000px

ACTUAL RESULTS: The new window is the size you set in step 4.

This is a regression from bug 1137009.  In particular, if I read the part 1 changeset correctly, we used to persist the size of windows in nsWebShellWindow::WindowActivated but now we don't do that; we just "save" it and never write it out unless the user then resizes the window the got activated... and then we only write it out for the dimension it's resized in.  As a result, we end up persisting transient window resizes and overwriting the user's preferred window size.  This makes it very difficult to maintain one's windows at the desired size.

The key part here is that when SaveAttributes() does the whole SetAttribute() thing, that no-ops very early if the value being set is the existing one, which means XULDocument::AttributeChanged is never called and the attributes are never persisted.  In particular, this makes the SaveAttributes call in nsWebShellWindow::WindowActivated never really do anything, and the same is likely true of the other callsites if the attribute hasn't actually changed.  Or put another way, the mPersistentAttributesDirty flags no longer guarantee things will actually be persisted.

Looks like this also affects Aurora, since we backported bug 1137009...

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Hmm... Okay, that describes why we persist the attributes twice in different places. Thanks for the explanation.

It seems to me this persisting mechanism is broken from the beginning with multi-window app. We did this trick on the window to make us always persist the dimensions of the last activated window, however attributes of widgets inside could still be inconsistent.

Anyway, if it is an important issue, I'd like to backout bug 1137009 (which has already been partially backouted in bug 1211344) and do it in the other way:
1. if the element is <xul:window>, skip persisting in AttributeChanged callback
2. if the window is in fullscreen, skip persisting in SavePersistentAttributes
Does this sound good to you?

I think we should have a test for this case from the beginning so that I wouldn't have broken it by mistake.

Comment 2

[Boris Zbarsky [:bzbarsky]]

>1. if the element is <xul:window>, skip persisting in AttributeChanged callback
>2. if the window is in fullscreen, skip persisting in SavePersistentAttributes

I don't know the persistence setup well enough to really evaluate this proposal, sorry...

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

So now bug 1137009 has been backed out. I'd like to leave this bug as-is for now, and write a test for it later.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

It looks like the patches in 1137009 were backed out in m-c and aurora. So 44 and 43 should not be affected by this regression now. 

I don't feel certain of that since the work in 1137009 seems intertwined with several other bugs. 

Tracking this since it was a regression and the work is ongoing. If this problem turns up again, can you set the status-firefox-44/43 bug back to "affected"? Thanks.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

I'm going to write a test for this bug to ensure it won't turn up again.

Comment 6

[Loic]

I filed bug 1264968 but maybe it's just another variant of this current bug, so let's dupe it if that's the case.

The regression introduced by bug 1137009 makes that the sizes of some windows in about:preferences are not saved in memory after the user changed them.

It's especially the case of the Saved Logins window, which is a feature loss for the user who wants to keep a large window to display his saved logins.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Loic from comment #6)
> I filed bug 1264968 but maybe it's just another variant of this current bug,
> so let's dupe it if that's the case.
> 
> The regression introduced by bug 1137009 makes that the sizes of some
> windows in about:preferences are not saved in memory after the user changed
> them.
> 
> It's especially the case of the Saved Logins window, which is a feature loss
> for the user who wants to keep a large window to display his saved logins.

No. This bug itself has been fixed long ago. It is still open just because I wanted to write a test for it, but I forgot to.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Do you think that bug 1264968 is related at all? 
If not then let's close this bug (maybe open a new bug for the tests)

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

No, that is not related at all. Hmmm, okay, let's close it.