Sometimes saves incorrect window position

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: Warner Young, Assigned: bz)

Tracking

({fixed1.5})

Trunk
x86
All
fixed1.5
Points:
---
Bug Flags:
blocking1.5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
Tested some more, and found that build 20030620 is okay, but 20030621 shows this
problem.
(Reporter)

Comment 2

15 years ago
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).
I doubt 190375 has changed anything do to with window positioning.
(Reporter)

Comment 4

15 years ago
Just checked on 2003062508 build of Mozilla, and it happens there, too. 
Changing product to Browser.
Component: General → XP Toolkit/Widgets
Product: Phoenix → Browser
Version: unspecified → Trunk
(Reporter)

Comment 5

15 years ago
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

15 years ago
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. 
(Reporter)

Comment 7

15 years ago
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

15 years ago
*** Bug 214662 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 9

15 years ago
Still occurs on build 2003 082708.

Updated

15 years ago
Flags: blocking1.5?

Comment 10

15 years ago
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

15 years ago
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

15 years ago
Ben or Blake, can you look at this? It's likely to be the kind of annoying bug
that drives people away. 
Flags: blocking1.5? → blocking1.5+

Comment 13

15 years ago
this was broken 22 june. taking.
Assignee: blake → danm-moz
Target Milestone: --- → mozilla1.6alpha

Comment 14

15 years ago
Created attachment 130869 [details] [diff] [review]
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.

Updated

15 years ago
Attachment #130869 - Flags: superreview?(dbaron)
Attachment #130869 - Flags: review?(bz-vacation)

Updated

15 years ago
Status: NEW → ASSIGNED
Target Milestone: mozilla1.6alpha → mozilla1.5beta
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?
Attachment #130869 - Flags: review?(bz-vacation) → review-

Comment 16

15 years ago
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.
Assignee: danm-moz → bz-vacation
Status: ASSIGNED → NEW
Target Milestone: mozilla1.5beta → ---

Comment 17

15 years ago
Created attachment 130918 [details] [diff] [review]
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.
> 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

15 years ago
Created attachment 130927 [details] [diff] [review]
explicitly persist window attributes

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

Updated

15 years ago
Attachment #130927 - Flags: review?(bz-vacation)
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...
Attachment #130927 - Flags: superreview?(dbaron)
Attachment #130927 - Flags: review?(bz-vacation)
Attachment #130927 - Flags: review+

Updated

15 years ago
Attachment #130869 - Flags: superreview?(dbaron)
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.
Attachment #130927 - Flags: superreview?(dbaron) → superreview+

Comment 22

15 years ago
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
}

Updated

15 years ago
Attachment #130869 - Attachment is obsolete: true

Updated

15 years ago
Attachment #130918 - Attachment is obsolete: true

Updated

15 years ago
Attachment #130927 - Flags: approval1.5?
I think the

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

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

Comment 24

15 years ago
Created attachment 131070 [details] [diff] [review]
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

15 years ago
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.
OS: Windows XP → All
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
Attachment #130927 - Flags: approval1.5? → approval1.5+
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
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Updated

15 years ago
Keywords: fixed1.5

Comment 28

15 years ago
Should we put this in 1.4?

Comment 29

15 years ago
The patch is safe enough. Aren't you guys done with 1.4 yet?
This isn't an issue on the 1.4 branch, since bug 209634 landed after 1.4 branched.

Comment 31

15 years ago
This behavior happens in the 1.4.1 builds. How can it not be an issue?
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.
You need to log in before you can comment on or make changes to this bug.