210689 - Sometimes saves incorrect window position

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Warner Young]

Possibly a dupe of bug 86955, except that this only started in the last couple
of days.

To Reproduce:
1) Open browser.  Open a 2nd browser window, which is usually offset from the
1st one slightly.  Close 2nd window.  Close 1st window.
2) Repeat.

Expected:
Each time you open the 1st browser window, it should open in the same position.

Actual results:
Sometimes, but not always, the 1st browser window opens at the position last
occupied by the 2nd window.

I haven't determined what causes this yet.  So far, it seems to be random.  I've
been seeing it with the 20030624 build of Moz Firebird on WinXP.  Previous
Firebird build I was using was 20030618, which didn't exhibit this problem.

Comment 1

[Warner Young]

Tested some more, and found that build 20030620 is okay, but 20030621 shows this
problem.

Comment 2

[Warner Young]

Given the timing of this bug, and looking at checkins, I'm wondering if it's due
to bug 190735.  CC'ing roc (I apologize in advance if this is not the right
thing to do).

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I doubt 190375 has changed anything do to with window positioning.

Comment 4

[Warner Young]

Just checked on 2003062508 build of Mozilla, and it happens there, too. 
Changing product to Browser.

Comment 5

[Warner Young]

Bug still occurs on 20030720 build of Firebird.  At first I thought I could work
around by setting middleclick to open new tabs instead of windows, but that
doesn't help in all cases.  If a link is set to open in a new window, or if
something on a page causes a popup, that destroys my saved window position info,
too.

I don't think I can justify raising the severity, but is there any chance
someone can track down the root cause soon?  It seems like it shouldn't be too
hard to find the cause, given that we know approximately when this bug was
introduced.  This bug currently makes Mozilla/Firebird extremely annoying to use
for me.

Comment 6

[hub-san]

I confirm this bug using 2003-07-24-03-trunk build of Mozilla on MacOSX10.2.6. 
Please change status of Hardware and OS, because I can't.  This bug can be
reproduced always on my machine.  Not sometimes.  Can you reproduce this bug
always or sometimes?  BTW, I'm getting annoyed by this bug recently.  I can't
use keyboard combination(Cmd+N or Ctrl+N) of opening new window because of this
bug.  If I use this shortcut, Mozilla opens 2nd window position next time. 

Comment 7

[Warner Young]

Still occurs on 2003-07-29-08 build.  Unfortunately, this bug is a blocker for
me, as I can't stand having to reposition my browser window after almost every
session.

Comment 8

[Pham]

*** Bug 214662 has been marked as a duplicate of this bug. ***

Comment 9

[Warner Young]

Still occurs on build 2003 082708.

Comment 10

[nomoreregistrations]

Since window position is a very valuable data to those of us with obsessive
tidiness disorder, doesn't this bug deserve "dataloss" keyword?

Comment 11

[dnapalm]

I don't know about dataloss, but I was in danger of sanity loss before I found
the workaround on the forums. It would be nice though if the closed window
position (not opened window position) was saved for the 1.5 and FB 0.7 releases,
or some sort of option added.

BTW the workaround, in case you didn't know, was to make localstore.rdf in your
profile directory read-only. Hope it doesn't cause problems, but it's been
working for me.

Comment 12

[Asa Dotzler [:asa]]

Ben or Blake, can you look at this? It's likely to be the kind of annoying bug
that drives people away. 

Comment 13

[Dan M]

this was broken 22 june. taking.

Comment 14

[Dan M]

Attachment: remove xul element attribute setting optimization

Once a year or so some bright fellow wants to optimize the setting of
attributes. This removes such an optimization and fixes this bug.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 130869 [details] [diff] [review]
remove xul element attribute setting optimization

This is not an optimization but a necessary correctness fix -- without this
code we incorrectly fire mutation events when no mutation occured.

What exactly is depending on these spurious mutation events and how can we fix
it to stop depending on them?

Comment 16

[Dan M]

Well, that's just swell.

The problem is, all nsXULDocuments compete for a single attribute persistence
store. In the following steps to reproduce (and by the way this happens every
time; it's not intermittent as the original steps claim)

1) Launch browser. It opens window w1.
2) File->New->Navigator Window. It opens window w2.
3) Close w2. w1 is activated.
4) Close w1. Mozilla quits.

In step 2 w2's position is cascaded. The values of the new (screenX,screenY)
attributes are correspondingly changed, and those values are correctly saved in
persistent store. In step 3, when w1 is re-activated it attempts to save its
unchanged positional attributes to persistent store but is stymied by the
new(ish) code.

We could possibly force the non-change to be noticed in this one specialized
case by doing something like
  SetAttr(value+1)
  SetAttr(value)
but that's an unhappy solution. I'm pretty sure it will cause a noticeable jump
in Txul times.

We may have a fundamental problem here. bz, you broke it, so I'm punting to you.

Comment 17

[Dan M]

Attachment: set persistent attribute values twice

This forces an attribute value update by the
  SetAttr(value+1)
  SetAttr(value)
method (see the above comment). It also works, but it's very inefficient and
will set the Txul monitors clanging.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> In step 3, when w1 is re-activated it attempts to save its
> unchanged positional attributes to persistent store

Perhaps we need to notify document observers for XUL but not fire mutation
events in these cases?

More importantly, is there a reason this code is not using
nsIDOMXULDocument::persist instead of setting attributes and assuming that that
will always trigger persistence?

I'm quite happy to fix this once I get back, but if we want it for 1.5 I'm not
the best owner in the world.

Comment 19

[Dan M]

Attachment: explicitly persist window attributes

Very well. This patch also fixes the bug, if that's the way to do it.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 130927 [details] [diff] [review]
explicitly persist window attributes

r=bzbarsky.  Thanks for looking into this, Dan!

On a separate note, we may want to change or augment this API to take an
nsIDOMElement instead of an element ID, since most of the callers seem to
actually have the DOM node, not just the ID.  Separate bug for that though, and
some email flogging for hyatt to look at it...

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 130927 [details] [diff] [review]
explicitly persist window attributes

>-  nsCOMPtr<nsIDOMElement> docShellElement;
>+  nsAutoString                windowElementId;
>+  nsCOMPtr<nsIDOMElement>     docShellElement;
>+  nsCOMPtr<nsIDOMXULDocument> ownerXULDoc;
>   GetWindowDOMElement(getter_AddRefs(docShellElement));
>-  if(!docShellElement)
>+  if (docShellElement) {
>+    // fetch docShellElement's ID and XUL owner document for later use
>+    nsCOMPtr<nsIDOMDocument> ownerDoc;
>+    docShellElement->GetOwnerDocument(getter_AddRefs(ownerDoc));
>+    ownerXULDoc = do_QueryInterface(ownerDoc);
>+    nsCOMPtr<nsIDOMXULElement> XULElement(do_QueryInterface(docShellElement));
>+    if (XULElement)
>+      XULElement->GetId(windowElementId);
>+ } else
>     return NS_ERROR_FAILURE;


No need to do

if (foo) {
  // lots of stuff
} else
  return NS_ERROR_FAILURE;

when you can just do

if (!foo)
  return NS_ERROR_FAILURE;

// lots of stuff

Then you won't even need to declare all the variables way in advance, although
it's probably better to do that anyway and scope the variables you don't need
later in a block to keep codesize down (by making the nsCOMPtr destructors
happen only in one place, at the end of the block), i.e.,:

if (!foo)
  return NS_ERROR_FAILURE

// variables you need later
{
  // stuff, including nsCOMPtrs you don't need later
} // nsCOMPtr destructors happen here, rather than at every early
  // return later in the function (although I admit there are only
  // three returns later in the function)

With that, sr=dbaron.

Comment 22

[Dan M]

But that was the point of my funny block.
if (!error) {
  initialize some gunk using scoped temporary variables
} else
  return

is pretty much the same thing as your suggestion,
if (error)
  return
{
  initialize some gunk using scoped temporary variables
}

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the

if (!error) {
  // ...
} else
  return

obfuscates the error handling bit, but I guess either is fine.

Comment 24

[Dan M]

Attachment: explicitly persist window attributes, better version

If we're anxious to see this fix in before the 1.5 branch is cut (will that be
soon, now?) someone who isn't me will need to check it in. I suggest this
version, which is just like the previous, reviewed one but now with 30% more
dbaron happiness.

Comment 25

[Felix Miata]

I mentioned this behavior to mkaply Friday, and he pointed me to this bug this
AM. This bug also applies to OS/2 vacpp builds, which include 1.4 and the 1.4.x
branch, because it's that old. I'm currently using Mozilla/5.0 (OS/2; U; Warp
4.5; en-US; rv:1.4.1) Gecko/20030904.

I wouldn't even know this behavior existed, were it not for the infuriating
behavior of 'Help -> About Mozilla' opening a new window. I can't remember ever
opening a new window on purpose, so about the only time it happens here is when
I need the build string to paste into a bug comment.

Here, my browser is virtually always at the same position, occupying the full
available vertical space, and leaving about 1/4" (on 19" CRT) of desktop showing
at right, and about 2" of desktop showing at left. The help about window always
opens at available 0,0, which is where the first window next opens on restart
about half the time.

FWIW, I keep CZ open virtually always, but only open mailnews as necessary, and
have never opened composer in my current profile. Usually I close CZ before
browser when shutting down to unzip a new nightly, but not always.

Comment 26

[Brendan Eich [:brendan]]

Comment on attachment 130927 [details] [diff] [review]
explicitly persist window attributes

Approved for 1.5, now to find danm and get him to check in.

/be

Comment 27

[Brendan Eich [:brendan]]

Re: comment 22:

if (!error) {
  initialize some gunk using scoped temporary variables
} else
  return;

when the rest of the method's non-error code follows the return; line is just
obfuscatory and confused.  Why isn't the entire method body in the "then" part?
 There's no good reason.  Control flow that consistently treats error cases with
abnormal early returns is clearer.  So is control flow that does not arbitrarily
divide the "normal" code into pieces just to put braces around some of it and
interject a misplaced error return in the middle.

Thus endeth today's sermon ;-).

I checked in the "better version" patch (attachment 131070 [details] [diff] [review]).  Thanks to danm for
the fix.

/be

Comment 28

[Mike Kaply [:mkaply]]

Should we put this in 1.4?

Comment 29

[Dan M]

The patch is safe enough. Aren't you guys done with 1.4 yet?

Comment 30

[Boris Zbarsky [:bzbarsky]]

This isn't an issue on the 1.4 branch, since bug 209634 landed after 1.4 branched.

Comment 31

[Felix Miata]

This behavior happens in the 1.4.1 builds. How can it not be an issue?

Comment 32

[Boris Zbarsky [:bzbarsky]]

If this happens on the 1.4 branch it's presumably for a reason different from
that which this patch fixed....  Again, the changes made in this patch were
necessitated by the fix for bug 209634, which is not in 1.4.x.