Closed Bug 924456 Opened 11 years ago Closed 5 years ago

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

Categories

(Core :: Widget: Cocoa, defect, P3)

Product:
Core
Component:
Widget: Cocoa
Type:
defect

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

(Whiteboard: tpi:+)

Attachments

(4 files, 1 obsolete file)

skeleton-0.1.xpi
(deleted), application/x-xpinstall
Details
skeleton-0.1-bdsf.xpi
(deleted), application/x-xpinstall
Details
Stack trace of this bug
(deleted), text/plain
Details
make MozAfterPaint fire when nsCocoaWindow::Show is no longer on the stack
(deleted), patch
Details | Diff | Splinter Review
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"
Blocks: 875157
Attached file skeleton-0.1.xpi (deleted) —
PI that shows the problem.
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
I'm testing on Mac and i just grabbed a nightly and it's failing for me. It's probably timing related.
Odd. The uploaded version is working, but my local copy isn't. But they are the same code. Checking.
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?
Attached file skeleton-0.1-bdsf.xpi (deleted) —
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
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.
(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.
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
(Question was for Mike)
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
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.
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)
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.
(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.
> 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.
> 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?
Attached file Stack trace of this bug (deleted) —
> 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)
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: fxdesktopbacklog
Severity: major → normal
OS: Mac OS X → All
Version: 24 Branch → Trunk
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)
Depends on: 974197
Removed from Backlog based on team feedback from the point estimation meeting.
No longer blocks: fxdesktopbacklog
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
ping. Can we have this fixed, please? This is messing up the UI events.
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
Whiteboard: tpi:?
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)
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)
Assignee: mstange → nobody
Flags: needinfo?(mstange)
Priority: P1 → P2
Moving to p3 because no activity for at least 1 year(s). See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Priority: P2 → P3
I'll take this.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED

(In reply to Mike Kaply [:mkaply] from comment #48)

I think this shows it had no effect.

Hi! I'm not sure what information is being asked for from me. Does the patch work, and you're wondering whether or not its acceptable to land given the Talos numbers, or does the patch not work and you're wondering why, or something else entirely?

Flags: needinfo?(mconley) → needinfo?(mozilla)

Sorry, forgot context.

Many ages ago in the patch:

Thanks, mkaply. Seems okay, but I'm wary of Talos regression sensitivity here. Can you do a try run and show a comparison against central?

and I failed to followthrough.

So this is me posting Talos and based on my limited understanding, I think it has no effect?

Flags: needinfo?(mozilla) → needinfo?(mconley)

Looks to me like we're in the noise. No strong signals, so let's go for it.

Flags: needinfo?(mconley)
Pushed by mozilla@kaply.com: https://hg.mozilla.org/integration/autoland/rev/83d45c7359ac Ensure that MozAfterPaint events are not fired before the window is completely visible. r=mconley

I think I'm going to just end this bug. It really only applied to legacy extensions, and I don't know what behavior would be affected now.

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(mozilla)
Resolution: --- → WONTFIX
Attachment #9022732 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: