598673 - Windows whose persisted locations are offscreen no longer show up

Status: RESOLVED WORKSFORME

(Core :: XUL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This is a regression from bug 574690 (verified via local bisect).

SYMPTOMS:  I recently ran into a situation where trying to open DOM Inspector silently did nothing.  Similar for view source.  I have since been able to produce a similar situation where trying to start the _browser_ silently does nothing.  I think we need to block on fixing this.

BUILD: Any build with the fix for bug 574690 in it (e.g. today's nightly).

STEPS TO REPRODUCE (using the view-source window):
1)  Produce a profile which has a localStore.rdf that has a persisted width for
    the view-source window of N px and a persisted screenX for the view-source
    window of M px such that M + N < 0.  See below on ways to do this.
2)  Start a browser using this profile
3)  Try to view source (using the menu, or Cmd-u)

EXPECTED RESULTS: view-source window shows up at left edge of screen (this is
                  the old behavior).

ACTUAL RESULTS: view-source window doesn't appear.  No errors in JS console. 
                In a debug build, I see:

  ###!!! ASSERTION: non-root frame's desired size changed during an incremental
  reflow: '(target == rootFrame && size.height == NS_UNCONSTRAINEDSIZE) ||
  (desiredSize.width == size.width && desiredSize.height == size.height)', file
  /Users/bzbarsky/mozilla/debug/mozilla/layout/base/nsPresShell.cpp, line 7491

DETAILS: It looks like the problem is that we position the window offscreen before showing, and then don't do anything to ensure it ends up on the screen or something.  This may be Mac-only.

HOW TO GENERATE A localStore.rdf THAT SHOWS THE PROBLEM:

Method 1: Start the browser with some profile, open the view-source window, quit, then hand-edit the localStore.rdf to have a large negative screenX (larger than the width) for the view-source window.  Example from my localStore.rdf:

    <RDF:Description 
       RDF:about="chrome://global/content/viewSource.xul#viewSource"
       width="640"
       height="480"
       sizemode="normal"
       screenX="-960"
       screenY="407" />
Method 2 (which normal users may hit, and which I hit initially):  Attach an external monitor to your laptop, with the monitor arrangement in OS preferences set such that the external monitor is to the left of the laptop's built-in monitor.  Start the browser.  Open the view-source window.  Move it left so that it is entirely on the external monitor.  Quit the browser.  Unplug the external monitor.  You should now have a localStore.rdf with a very negative screenX.

Comment 1

[Shawn Wilsher :sdwilsh]

Given that windows don't even show up without any feedback, I'm going to say that this blocks the final release.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Fwiw, my gut feeling is that a fix will need beta-shaking-out, but maybe Jim can think of something safe.

Comment 3

[Benjamin Smedberg]

What's the desired behavior? If a window restores offscreen, do we move it so that part of it is onscreen, or reset it to the default window position?

Comment 4

[Boris Zbarsky [:bzbarsky]]

The old behavior was to move it so that it's fully onscreen (so in particular to put it at screenX == 0).  That may have been an accident, though.

Also, the old behavior did NOT change the value stored in localStore, so if you didn't move the window, then closed it and plugged in the external monitor and opened the window again it would show at the "right" place on the external monitor.  Again, that may or may not be desired.

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> Fwiw, my gut feeling is that a fix will need beta-shaking-out, but maybe Jim
> can think of something safe.
Hmm, so I was making the (possibly poor) assumption that we'd just reset it to the default window position, which I wouldn't think would need beta feedback.  I guess getting an answer to comment 3 may change what it blocks.

Comment 6

[Boris Zbarsky [:bzbarsky]]

To be clear, it's the final patch for bug 574690 (the order-changing) that causes this problem.

Comment 7

[Boris Zbarsky [:bzbarsky]]

And specifically, the nightly regression range is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5fda39cd703c&tochange=96de199027d7

Comment 8

[Jim Mathies [:jimm]]

On windows this is prevented by ConstrainPosition, which is called from nsXULWindow:

http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.cpp#1109

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#1656

Maybe simplest fix here is to implement this for cocoa?

http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsCocoaWindow.mm#980

Comment 9

[Boris Zbarsky [:bzbarsky]]

See, that's the sort of thing that would need a beta shakeout... ;)

But yes, that might work.

Comment 10

[Jim Mathies [:jimm]]

Since this isn't implemented, I wonder how are managed to deal with this in 3.6?

Comment 11

[Boris Zbarsky [:bzbarsky]]

The window manager may have handled it for us....

Comment 12

[Jim Mathies [:jimm]]

Would someone who regularly develops on the mac like to take this? I recent picked up a mini but it's not setup yet. I can try and implement this I suppose but it's going to be last on my blocker list.

Comment 15

[Wladimir Palant]

Is bug 591973 the Windows variant of the same issue? While the window there isn't entirely invisible - it's close enough that some users don't notice it and think that their browser is hanging.

Comment 16

[Johnathan Nightingale [:johnath]]

Adding a couple mac brains.

Comment 17

[Jim Mathies [:jimm]]

If we can't fix this by implementing ConstrainPosition or similar in mac widget code, I can back out the changes in bug 574690 and put together a windows widget fix of some sort. We could also do some ifdefing in nsXulWindow so as to keep the old behavior on macs. It would be nice though if both widget libraries supported making sure windows get created the right way despite changes like this to common code.

Comment 18

[Dave Miller [:justdave]]

I just ran into a similar issue as a result of a botched session restore after a crash...  Rather than being placed offscreen, though, the main browser window somehow got resized to 1x23 pixels, making it impossible to click on or resize.  Had to fix it by editing the Width and Height parameters for the window in the sessionstore.js file while the browser wasn't running.

Comment 19

[Jim Mathies [:jimm]]

The patch that caused this (bug 574690) has been backed out. Should be fixed in tomorrow's nightly.

Comment 20

[Jim Mathies [:jimm]]

I'm going to go ahead and close this. The offending patch was backed our, and bz was quit confident in comment 1 that that was the cause.

Comment 22

[Wladimir Palant]

Confirmed, this works for me now (Mienfield build 20101017).

Comment 23

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336