MozAfterPaint might fire too early on Mac, meaning browser-delayed-startup-finished fires too early on Mac, meaning I can't show dialog

NEW
Unassigned

Status

()

Core
Widget: Cocoa
P2
normal
4 years ago
6 months ago

People

(Reporter: mkaply, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: tpi:+)

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
The documentation of sessionstore-windows-restored says:

Sent by the session restore process to indicate that all initial browser windows have opened. Note that while the window are open and the chrome loaded the tabs in the windows may still be being restored after this notification.

This would indicate that the window is usable to display a message.

If you call:

Services.wm.getMostRecentWindow("navigator:browser");

and then use that window to display a prompt, you get:

"Cannot call openModalWindow on a hidden window"
(Reporter)

Updated

4 years ago
Blocks: 875157
(Reporter)

Comment 1

4 years ago
Created attachment 814440 [details]
skeleton-0.1.xpi

XPI that shows the problem.
(Reporter)

Comment 2

4 years ago
I also tried using browser-delayed-startup-finished which per the docs is:

Sent when the browser window and all its components have been loaded and initialized.

But again, no luck. I checked and isParentWindowMainWidgetVisible is false on the window.
That skeleton XPI works for me in a Nightly Mac build. I get a prompt after startup and no exception thrown.
Component: Document Navigation → General
Product: Core → Firefox
(Reporter)

Comment 4

4 years ago
I'm testing on Mac and i just grabbed a nightly and it's failing for me.

It's probably timing related.
(Reporter)

Comment 5

4 years ago
Odd. The uploaded version is working, but my local copy isn't. But they are the same code. Checking.
(Reporter)

Comment 6

4 years ago
Argh. It does working on nightly. I had changed to browser-delayed-startup-finished to test that (it still doesn't work).

Any ideas what fixed this?
Created attachment 814647 [details]
skeleton-0.1-bdsf.xpi

I can reproduce with this version (which uses browser-delayed-startup-finished).
browser-delayed-startup-finished is notified synchronously from the chrome window's MozAfterPaint handler.
Summary: Cannot display a prompt during processing of sessionstore-windows-restored → cannot display a prompt during browser-delayed-startup-finished
(Reporter)

Comment 9

4 years ago
As an aside, the reason the default browser prompt didn't run into this is because even though they do the ask on sessionstore-windows-restored, they do it via Services.tm.mainThread.dispatch.

Would that be recommended for any messages during startup?
sessionstore-windows-restored works in all cases, right? This bug is about browser-delayed-startup-finished, I thought, per comment 6.
(Reporter)

Comment 11

4 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #10)
> sessionstore-windows-restored works in all cases, right? This bug is about
> browser-delayed-startup-finished, I thought, per comment 6.

It didn't work in Firefox 24, and since that's ESR, I'll be supporting it for a year.

Comment 12

4 years ago
Gavin, it always felt wrong to me to use an event [name] like "sessionstore-windows-restored" to know "Firefox is started". This event is so basic, it shouldn't have anything to do with session restore, which is an implementation detail.

So, what's the status of
1) "browser-delayed-startup-finished"
2) "sessionstore-windows-restored"
in
1) FF17 ESR
2) FF24 ESR
3) FF25
(matrix, i.e. 6 results)?
Version: Trunk → 24 Branch

Comment 13

4 years ago
(Question was for Mike)
(Reporter)

Comment 14

4 years ago
sessionstore-windows-restored
Firefox 17 - Works - displays over window
Firefox 24 - Doesn’t work - Cannot call openModalWindow on a hidden window
Firefox 25 - Works - displays over window

browser-delayed-startup-finished
Firefox 17 - Works - displays over window
Firefox 24 - Doesn’t work - Cannot call openModalWindow on a hidden window
Firefox 25 - Doesn’t work - Cannot call openModalWindow on a hidden window

Comment 15

4 years ago
Thanks for the tests.

> browser-delayed-startup-finished
> Firefox 24 - Doesn’t work - Cannot call openModalWindow on a hidden window
> Firefox 25 - Doesn’t work - Cannot call openModalWindow on a hidden window

OK, that's bad.

Updated

4 years ago
Severity: normal → major
Mark, can you debug this further?
Assignee: nobody → mhammond
(In reply to Mike Kaply (:mkaply) from comment #14)
> browser-delayed-startup-finished
> Firefox 17 - Works - displays over window
> Firefox 24 - Doesn’t work - Cannot call openModalWindow on a hidden window
> Firefox 25 - Doesn’t work - Cannot call openModalWindow on a hidden window

I can't reproduce this - on 24, 25 and trunk using Gavin's xpi on Windows 7, I see the alert as the browser starts.  Mike, is that the .xpi you were using?
Flags: needinfo?(mozilla)
(Reporter)

Comment 18

4 years ago
Looks like it's Mac only.
Flags: needinfo?(mozilla)
OS: All → Mac OS X
The problem seems to be that although nsCocoaWindow::Show() has been called, nsCocoaWindow::IsVisible() is returning false.  Another method, IsVisibleOrBeingShown() exists in the cocoa widget and this returns true, which seems to confirm the window is in the process of being shown.

For the purposes of this check, IsVisibleOrBeingShown() sounds like what we want - but sadly it isn't exposed to nsWidget.  Also, moving the NotifyObservers() call into a setTimeout(..., 0); also makes things work, but (a) that might not be deterministic and (b) that might not be desirable for other reasons.

Can anyone recommend someone who understands Cocoa and we could rope in for a discussion?
smichaud is my go-to Cocoa specialist.
Flags: needinfo?(smichaud)
(In reply to Mike Conley (:mconley) from comment #20)
> smichaud is my go-to Cocoa specialist.

Thanks!  The tl;dr version of this bug is:

browser.js sends an observer notification at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1179.  An |alert| call during this notification fails to show, but instead dumps a message to the console about the window being hidden.

The "is window hidden" check is in nsPrompter.js, and ends up calling http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp#3279.  This function ends with |*aIsVisible = parentWidget->IsVisible();| - see comment 19 - IsVisible() returns false, but IsVisibleOrBeingShown() returns true, reflecting the fact that ::Show() *has* already been called.

The easiest way to reproduce is to modify browser.js, and add a simple |alert| call just before (or after) that observer notification is sent.  Alternatively, install the .xpi uploaded by Gavin, which simply registers an observer for that notification and attempts an alert.
I'll take a look at this in the next day or two.  But I'll say this much now:

We *don't* want to display a modal window (as opposed to a sheet) on OS X.  Modal windows are problemmatic on OS X because they must always be app-modal, but our implementation of JavaScript's showModalWindow pretends that's not true (with disastrous results, e.g. bug 476541).  For more information see bug 478073, bug 729720 and bug 395465.

A sheet displayed above an invisible window won't work.

If this window/dialog truly needs to be displayed "above" an invisible window, we should try as hard as we can to make it non-modal.
Flags: needinfo?(smichaud)
> A sheet displayed above an invisible window won't work.

Is that true even if nsCocoaWindow::Show() has been called for that window?

If nsCocoaWindow::Show() has been called, is there any guarantee about when IsVisible() would return true?  ie, would a single event-loop spin be enough?
Adding back the needinfo flag to remind me.

I won't be able to answer questions like Mark's here until I've dug into this quite a lot further.  With luck I'll be able to find a way to avoid using a modal window.
Flags: needinfo?(smichaud)
For some additional context, this bug was introduced when we added the "is window hidden" check - previously no such check was made and things worked fine.  However, we added the check because we do *not* want the alert to work when the window really isn't visible.

So in this case, the window *reports* it isn't visible, but logically it *is* visible, as the |alert| would have worked if not for the new IsVisible() check.
(Reporter)

Comment 26

4 years ago
(In reply to Mark Hammond [:markh] from comment #25)
> However, we added the check because we do *not* want the alert to work when the window really isn't visible.

I never saw a good explanation for the "why" of this change. What was especially confusing is that it was made as part of a change to background thumbnailing. Was anything actually broken because of it?

And to be clear, before the "is window hidden" check, it displayed in the upper right. Not the greatest UI, but it worked.
(In reply to Mike Kaply (:mkaply) from comment #26)
> I never saw a good explanation for the "why" of this change. What was
> especially confusing is that it was made as part of a change to background
> thumbnailing. Was anything actually broken because of it?

It was done to bring nsPrompter.js into line with the window watcher's method for opening a new window, which is used by the prompt service to open dialogs - http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#647

We could revisit this, but it seems a worthwhile goal that non-visible windows can't open other windows or alerts as the user will have no idea where they came from.

Comment 28

4 years ago
> it seems a worthwhile goal that non-visible windows can't open other windows or alerts as the user will have no idea where they came from.

Yes. But only if it's guaranteed that we always have a Firefox window, which is a reasonable expectation for code (including FF extensions). Given that this isn't true on Mac (and Mac only), this is a cross-platform pitfall, esp. if I don't have a Mac and don't expect this.
By the way, I've finally finished with bug 925448 and should be able to start on this tomorrow.
(In reply to Ben Bucksch (:BenB) from comment #28)
> > it seems a worthwhile goal that non-visible windows can't open other windows or alerts as the user will have no idea where they came from.
> 
> Yes. But only if it's guaranteed that we always have a Firefox window, which
> is a reasonable expectation for code (including FF extensions). Given that
> this isn't true on Mac (and Mac only), this is a cross-platform pitfall,
> esp. if I don't have a Mac and don't expect this.

FTR, the functions in nsPrompter.js all take a "domWin" as a param.  If this isn't specified, then we *do* show the prompt - so it's only a problem when domWin *is* specified and it is hidden.  In this specific |alert| case though, the caller of |alert| has domWin passed implicitly - but in the general case, I think that's still OK - if a (really) hidden window has content that calls |alert()|, it's fine (IMO) for it to be suppressed - but the fact the main window reports it *is* hidden even after |Show()| has been called is the fly in the ointment.

Comment 31

4 years ago
> the fact the main window reports it *is* hidden even after |Show()| has been called is the
> [problem]

Agreed.
(In reply to Mark Hammond [:markh] from comment #21)
> (In reply to Mike Conley (:mconley) from comment #20)
> > smichaud is my go-to Cocoa specialist.
> 
> Thanks!  The tl;dr version of this bug is:
> 
> browser.js sends an observer notification at
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> js#1179.  An |alert| call during this notification fails to show, but
> instead dumps a message to the console about the window being hidden.
> 
> The "is window hidden" check is in nsPrompter.js, and ends up calling
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.
> cpp#3279.  This function ends with |*aIsVisible =
> parentWidget->IsVisible();| - see comment 19 - IsVisible() returns false,
> but IsVisibleOrBeingShown() returns true, reflecting the fact that ::Show()
> *has* already been called.

At has been called, but it has not returned yet! That's the fundamental difference between isVisible and isVisibleOrBeingShown: isVisibleOrBeingShown additionally returns YES if we're currently inside nsCocoaWindow::Show.
I don't think it's a good idea to send a notification to JS before the construction of the native window has finished. Can we post a runnable first, and then fire the notification from that runnable?
Created attachment 8338807 [details]
Stack trace of this bug

> It [nsCocoaWindow::Show()] has been called, but it has not returned yet!

It's worse than that -- the call to nsDOMWindowUtils::GetIsParentWindowMainWidgetVisible() takes place while the call to nsCocoaWindow::Show() is still on the stack!

This nasty behavior is triggered here (by a call to nsViewManager::ProcessPendingUpdates()):

https://hg.mozilla.org/mozilla-central/annotate/34f431863037/view/src/nsViewManager.cpp#l657

I'll need to think about how best to work around this.  I'm not at all sure we'll be able to get out of this by using a Gecko runnable.  We *might*, though, be able to get the call to nsChildView::WillPaintWindow() from -[ChildView viewWillDraw] to run on the next iteration of the native runloop (for example by calling -[NSObject performSelector:withObject:afterDelay:0]).

(Sorry by the way for the mangled symbols.  I haven't yet figured out how to get my interpose library to unmangle them.)
I'm clearing the needinfo flag, but I'll keep working on this.

However I'll be gone the rest of this week, so I may not have anything more to say until next week.
Flags: needinfo?(smichaud)
Created attachment 8339251 [details] [diff] [review]
make MozAfterPaint fire when nsCocoaWindow::Show is no longer on the stack

I haven't tested this yet, but I think this should fix it. It's not clear to me whether this is a good way to fix it, though, because I think this makes delayedStartup run after the first paint, and I think at the moment it runs before the first paint.
(In reply to Markus Stange [:mstange] from comment #35)
> I haven't tested this yet, but I think this should fix it. It's not clear to
> me whether this is a good way to fix it, though, because I think this makes
> delayedStartup run after the first paint, and I think at the moment it runs
> before the first paint.

The intention is that it run after the first paint (that's what the event name implies, see also bug 715402), so that it's not currently doing so on Mac is a bug we should definitely fix.
Blocks: 950073
Severity: major → normal
OS: Mac OS X → All
Version: 24 Branch → Trunk

Updated

4 years ago
Whiteboard: p=0
Markus, that patch fixes the symptoms I noticed in https://bugzilla.mozilla.org/show_bug.cgi?id=955950#c26.

Can you take this bug and get it reviewed, assuming it's ready?
Component: General → Widget: Cocoa
Flags: needinfo?(mstange)
Product: Firefox → Core
Summary: cannot display a prompt during browser-delayed-startup-finished → browser-delayed-startup-finished fires too early on Mac (can't show dialog that early, "reset firefox" notification never appears)
Specifically, the symptoms I observed were that RecentWindow.jsm's getMostRecentBrowserWindow returns null on Mac from under browser-delayed-startup-finished, because Service.wm.getZOrderDOMWindowEnumerator returns an empty enumerator. Services.wm.getMostRecentWindow at that point does work, though, so the reset notification being broken was probably regressed by bug 891194.
Blocks: 498181
Hardware: x86 → All
Bug 974197 may also fix this. But the patch here is probably worth taking anyway, in some form. I'll take care of it.
Assignee: mhammond → mstange
Flags: needinfo?(mstange)

Updated

4 years ago
Depends on: 974197
Removed from Backlog based on team feedback from the point estimation meeting.
No longer blocks: 950073
Flags: firefox-backlog-
Whiteboard: p=0
Summary: browser-delayed-startup-finished fires too early on Mac (can't show dialog that early, "reset firefox" notification never appears) → MozAfterPaint might fire too early on Mac

Comment 41

2 years ago
ping. Can we have this fixed, please? This is messing up the UI events.

Updated

2 years ago
Summary: MozAfterPaint might fire too early on Mac → MozAfterPaint might fire too early on Mac, meaning browser-delayed-startup-finished fires too early on Mac, meaning I can't show dialog

Updated

2 years ago
Whiteboard: tpi:?

Comment 42

2 years ago
hey mike, is this something that might have been fixed by the MozAfterPaint event work for e10s?
Flags: needinfo?(mconley)
(In reply to Jim Mathies [:jimm] from comment #42)
> hey mike, is this something that might have been fixed by the MozAfterPaint
> event work for e10s?

I don't believe so, no.

If this is still an issue, mstange's patch here to block script until the window is visible is still probably the way to go.
Flags: needinfo?(mconley)

Updated

2 years ago
Priority: -- → P1
Whiteboard: tpi:? → tpi:+
Hi Markus, are you still planning to take care of this (comment 39), or should we set this back to unassigned?
Flags: needinfo?(mstange)

Updated

6 months ago
Assignee: mstange → nobody
Flags: needinfo?(mstange)

Updated

6 months ago
Priority: P1 → P2
You need to log in before you can comment on or make changes to this bug.