User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021029 Chimera/0.5+ Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20021029 Chimera/0.5+ Having a sheet displayed on one window causes typing in other windows to be backwards. Really! Reproducible: Always Steps to Reproduce: 1. Open two windows. 2. In window A, go to www.yahoo.com. 3. In window B, in the location bar, type "javascript:alert("blah");" and press return. That window will get a sheet. Leave the sheet. 4. Return to window A. In the search field, type "asdf". Actual Results: Each key press takes inordinately long to process, plus each is inserted into the field at the front, yielding a backwards series of letters. Expected Results: Typing should go just as fast as with no sheet, and should be forwards, not backwards.

The waiting for the keystrokes is apparently an event problem, as the wait goes away if you wiggle the mouse. Hmmm...

Confirmed using Chimera/2002110804 on 10.1.5. It WorksForMe using FizzillaCFM/2002110808. (See also his related bug 179306.)

Yikes!

Using 2004112308. This is weird. It's different with different types of sheets. If I use the javascript example, I get the weird typing business. However, if I use a Search The Web sheet, it doesn't happen. In other words, remove the Google field from your toolbar, and type Command-Shift-F in Window B. Go back to Window A with Yahoo, and typing in the field works as expected. I have no idea what that means.

Could this be the same as 269973, which is apparently the same as 234729, 260772, and 265001? Also 272723? Can someone decide which of these is the "real" bug? ;)

This still happens. Weird!

moving to 1.0 since its a sync vs. async api problem and we're simply not going to solve this any time soon

Also note that all buttons (bookmarks, back...) in the window do not work.

(In reply to comment #8) > Also note that all buttons (bookmarks, back...) in the window do not work. _That_ is bug 179306, which was closed as "that's just the way it is". However, FireFox has no problem loading pages when a window has a sheet on it, so I have some hope (however unjustified) that it will work someday.

I figured out the reason for the backwards typing. When any window has a gecko-created sheet up (e.g. from javascript::alert()), we're running in a modal event loop, with nsGlobalWindow stuff is on the stack. Now, when typing in another window, typing comes out backwards because the editor fails to move the selection when you type each character. The reason this fails is that we fail a security check, preventing the editor code from accessing nsIDOMRange methods. This is (I presume) because there is some "content" code on the stack. Now, Camino is doing something a bit skanky in the first place by showing modal gecko dialogs as sheets (thus allowing users to interact with other windows). Ideally, gecko would provide a non-blocking API for all callbacks that need to show dialogs. However, it seems odd that we're able to have almost fully functional other windows while in this state, but fall down on some small selection issue. I'm not sure how to proceed to fix this issue.

(In reply to comment #10) > Now, Camino is doing something a bit skanky in the first place by showing modal > gecko dialogs as sheets (thus allowing users to interact with other windows). Skankier than FireFox, which does the same thing? Of course, this isn't new; this was probably the cause of bug 179306 (which mysteriously fixed itself). > Ideally, gecko would provide a non-blocking API for all callbacks that need to > show dialogs. Which would fix things for FireFox too. BTW, try this bug on FireFox. You don't have the same problem, but then again, it won't let you switch back to the other window. (Yes, that should be filed as a different bug.) > I'm not sure how to proceed to fix this issue. Create non-blocking UI?

(In reply to comment #11) > (In reply to comment #10) > > I'm not sure how to proceed to fix this issue. > > Create non-blocking UI? That's well-nigh impossible. Every up-caller, including gecko, JS, security etc would have to be changed to use sheet-friendly APIs, which would mean making them non-blocking and stateful. And in some cases (e.g. javascript:alert()) I don't see how that can be done. I'm not sure why Firefox doesn't show the issue. The situation is very much the same.

We will have to move from sheets to application-modal dialogs to fix this.

So... this is a "known issue" (or rather known design situation). The reason Firefox doesn't show the problem is the code at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/appshell/src/nsXULWindow.cpp&rev=1.161&mark=391-393,406-408#391 which makes it explicitly look like no JS is on the stack. Anyone spinning any event loop MUST do this to avoid issues like this and some security issues. We currently have several bugs along those lines... I assume in this case the event loop is being spun somewhere in Camino code, right?

(In reply to comment #14) > Anyone spinning any event loop MUST do this to avoid issues like this and some > security issues. We currently have several bugs along those lines... > > I assume in this case the event loop is being spun somewhere in Camino code, > right? Right, at all the [NSApp runModalForWindow:...] calls here: http://lxr.mozilla.org/mozilla/source/embedding/browser/cocoa/src/nsAlertController.mm We should probably look into this for 1.0 for security reasons.

This patch adds a Push/Pop on the JSContextStack around our sheet/dialog display, which fixes the typing problem. However, this doesn't catch all the callers of runModalForWindow:(relativeToWindow:); the correct approach might be to subclass NSApplication, and override those two methods, doing the push/pop in there. bz: I presume that this push/pop is only necessary when our modal dialog/sheet is invoked via JS? Why doesn't the prompt service code do this for us?

Other places that may need fixing: SecurityDialogs::ConfirmDownloadCACert SecurityDialogs::SetPKCS12FilePassword SecurityDialogs::ConfirmUnknownIssuer SecurityDialogs::ConfirmMismatchDomain SecurityDialogs::ConfirmCertExpired SecurityDialogs::SetPassword SecurityDialogs::ChooseCertificate SecurityDialogs::DisplayGeneratingKeypairInfo Not sure about the KeychainService methods that show dialogs (which are triggered from a nsIFormSubmitObserver).

> bz: I presume that this push/pop is only necessary when our modal dialog/sheet > is invoked via JS? Or via C++ ultimately called from JS, yeah. > Why doesn't the prompt service code do this for us? Good question! For 1.9 we should try to fix this; there is supposed to be a thread somewhere on doing it better....

I pushed the nsIJSContextStack calls into a class method on nsAlertController, and am calling that from every place we get an upcall to show UI from Gecko.

Uh, the patch has a typo in +safeRunModalForWindow:relativeToWindow: and needs a project include path change to build, but you get the idea.

Comment on attachment 210326 [details] [diff] [review] Patch +struct JSContext; // allow nsIJSContextStack to be included without sucking in JS headers I don't think this is the typo you meant (inDialog/inWindow) - shouldn't this be a typedef? In safeRunModalForWindow:relativeToWindow:, you're popping even if you couldn't push. I know, Firefox does it too, but if Firefox jumped off a bridge, would you? Now we've got -runModalWindow:relativeToWindow: and +safeRunModalForWindow:relativeToWindow:, which are mostly the same except one's expected to be called on ObjC and raises exceptions, and the other insulates the caller from the exceptions and plays with the JS stack. Can you reduce the duplication?

(In reply to comment #21) > (From update of attachment 210326 [details] [diff] [review] [edit]) > +struct JSContext; // allow nsIJSContextStack to be included without sucking in > JS headers > > I don't think this is the typo you meant (inDialog/inWindow) - shouldn't this > be a typedef? No, it's just a forward-declaraction of the struct. > In safeRunModalForWindow:relativeToWindow:, you're popping even if you couldn't > push. I know, Firefox does it too, but if Firefox jumped off a bridge, would > you? I'll fix that. > Now we've got -runModalWindow:relativeToWindow: and > +safeRunModalForWindow:relativeToWindow:, which are mostly the same except > one's expected to be called on ObjC and raises exceptions, and the other > insulates the caller from the exceptions and plays with the JS stack. Can you > reduce the duplication? Well, the -runModalWindow:relativeToWindow: does the throw-on-error thing which +safeRunModalForWindow:relativeToWindow: should not do. I don't think it's too bad.

But neither you nor nsIJSContextStack.h use |struct JSContext*|, you use |JSContext|.

|JSContext*|

(In reply to comment #24) > |JSContext*| Right; we only every deal in pointers to this thing, so the compiler doesn't care what it is; that's why the forward declare works.

Forward declaration is fine, but why aren't you declaring the typedef (as you use it)? This works: typedef struct JSContext JSContext; JSContext* cx; This works: struct JSContext; struct JSContext* cx; This should not work, but it's what's in your patch: struct JSContext; JSContext* cx;

Duh, because it's Obj-C++ and not C.

Modified version of the patch checked in.

This is my attempt at converting Simon's checking to a usable patch for the 1.7 branch (for Camino 0.8.5). Note a couple of things: 1) I've never merged changes to another branch before. 2) I have not tested this patch and have *no way* of testing it as I don't have access to a 10.3 machine. 3) This patch only touches nsAlertController.h and nsAlertController.mm. Simon's checkin touched SecurityDialogs.mm, however I think that was mostly fixing warnings and not related to this patch. That file is completely different on the 1.7 branch and nothing made sense to merge. The original checkin also touched project.pbxproj. I'm not sure of what those changes were for so I didn't include them here. Mark, maybe you know if those changes were necessary... ? Again, I think they related to dialog changes. So... questions? Comments? Mark, I'm putting you down for review to make sure I did this right. Then we'll take it from there...

Real patch with fixed line endings.

Hopefully the last time...

The patch above *will* fail to build. The changes to the project file were necessary. Adding "../dist/include/xpconnect" to the list of HEADER_SEARCH_PATHS will fix most but not all of the errors. The final one, I can't seem to figure out why it's being caused. :-/

This is the Core change that mento did that allows the 1.7 branch version of this patch to build. (Note also the project patch is from Sam; I'm just attaching.) With these three patches, Camino-1.7branch builds and the backwards-typing is gone.

Comment on attachment 213629 [details] [diff] [review] 1.7 Branch Core change that gets it to actually build r=me, requesting a1.7.13 for a simple core change needed for Camino 0.8.5. We rely on nsServiceManagerUtils.h in Camino now in a patch that we want to take for 0.8.5. This change adds that header to SDK_HEADERS. nsServiceManagerUtils.h is in SDK_HEADERS on the trunk and 1.8 branches. If you don't want to add nsServiceManagerUtils.h to the SDK on the 1.7 branch, an alternative is to add it to EXPORTS in xpcom/glue/Makefile.in. We need to do one or the other.

Comment on attachment 213629 [details] [diff] [review] 1.7 Branch Core change that gets it to actually build Forget it. The right thing to do on the 1.7 branch is to: #include "nsIServiceManager.h" which should bring in nsIServiceManagerUtils.h, which is what we really need.

This a unified patch for 1.7 branch (Sam's patch + mento's change in comment 36, sam's project changes); builds, and backwards-typing no longer occurs.

Comment on attachment 213639 [details] [diff] [review] Final (?) patch for the 1.7branch This doesn't actually do anything without the nsAlertController portion of bug 314072. You've got +safeRunModalForWindow:relativeToWindow: doing the JSContextStack push/pop, which is only called by -runModalWindow:relativeToWindow:, but who calls that? Smokey, were you actually able to duplicate backwards typing in an unpatched 0.8.x build?

(In reply to comment #38) > Smokey, were you actually able to duplicate backwards typing in an unpatched > 0.8.x build? Yes, in 0.8.4.

Mark?... (Also, removing CCs from those in core who don't need more bugspam.)

Comment on attachment 213639 [details] [diff] [review] Final (?) patch for the 1.7branch I don't see how this patch works on its own. Nobody ever calls runModalWindow:relativeToWindow:. (And it's never declared before any callers would use it, either.) And that method is the only thing that ever calls safeRunModalForWindow:relativeToWindow: It's just dead code. See what I mean?

(In reply to comment #41) > (From update of attachment 213639 [details] [diff] [review] [edit]) > I don't see how this patch works on its own. Nobody ever calls > runModalWindow:relativeToWindow:. (And it's never declared before any callers > would use it, either.) And that method is the only thing that ever calls > safeRunModalForWindow:relativeToWindow: It's just dead code. See what I mean? > I'm not sure how it works, but I know I can reproduce the bug using the 0.8.5 nightly from today and Smokey says he can no longer reproduce it using a patched version. I certainly see what you're saying though. Do the "if (inParentWindow)" changes do something to fix it? Those seem like the most likely suspects.

(In reply to comment #42) > I'm not sure how it works, but I know I can reproduce the bug using the 0.8.5 > nightly from today and Smokey says he can no longer reproduce it using a > patched version. Smokey's smoking something. To be sure, I tested this patch on a tinderbox and it doesn't fix squat. Like I said, portions of bug 314072 need to be brought in. I'll just go ahead and do that now, since I'm already on the tinderbox anyway.

(Tested) Includes [NSApp runModalForWindow]->[self runModalWindow] changes from bug 314072.

Comment on attachment 214269 [details] [diff] [review] Working 1.7 branch patch That looks more like it.

Checkered in to 1_7.

v all around.