179307 - Sheet being displayed causes backwards typing in other windows' form fields

Status: VERIFIED FIXED

(Camino Graveyard :: HTML Form Controls, defect, P1)

Description

[Avi Drissman]

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.

Comment 1

[Avi Drissman]

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

Comment 2

[Greg K.]

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

Comment 3

[Simon Fraser [no longer active]]

Yikes!

Comment 4

[Joe Chellman]

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.

Comment 5

[Kendall Lister]

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? ;)

Comment 6

[Simon Fraser [no longer active]]

This still happens. Weird!

Comment 7

[Josh Aas]

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

Comment 8

[markturner]

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

Comment 9

[Avi Drissman]

(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.

Comment 10

[Simon Fraser [no longer active]]

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.

Comment 11

[Avi Drissman]

(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?

Comment 12

[Simon Fraser [no longer active]]

(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.

Comment 13

[Simon Fraser [no longer active]]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

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?

Comment 15

[Simon Fraser [no longer active]]

(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.

Comment 16

[Simon Fraser [no longer active]]

Attachment: WIP patch

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?

Comment 17

[Simon Fraser [no longer active]]

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).

Comment 18

[Boris Zbarsky [:bzbarsky]]

> 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....

Comment 19

[Simon Fraser [no longer active]]

Attachment: Patch

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.

Comment 20

[Simon Fraser [no longer active]]

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

Comment 21

[Mark Mentovai]

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?

Comment 22

[Simon Fraser [no longer active]]

(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.

Comment 23

[Mark Mentovai]

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

Comment 24

[Mark Mentovai]

|JSContext*|

Comment 25

[Simon Fraser [no longer active]]

(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.

Comment 26

[Mark Mentovai]

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;

Comment 27

[Mark Mentovai]

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

Comment 28

[Simon Fraser [no longer active]]

Modified version of the patch checked in.

Comment 29

[Samuel Sidler (old account; do not CC)]

Attachment: Patch for 1.7 Branch

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...

Comment 30

[Samuel Sidler (old account; do not CC)]

Attachment: Real Patch for 1.7 Branch

Real patch with fixed line endings.

Comment 31

[Samuel Sidler (old account; do not CC)]

Attachment: Rea Real Patch for Mozilla 1.7

Hopefully the last time...

Comment 32

[Samuel Sidler (old account; do not CC)]

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. :-/

Comment 33

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: project changes for 1.7 branch

Comment 34

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: 1.7 Branch Core change that gets it to actually build

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 35

[Mark Mentovai]

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 36

[Mark Mentovai]

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.

Comment 37

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Final (?) patch for the 1.7branch

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 38

[Mark Mentovai]

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?

Comment 39

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(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.

Comment 40

[Samuel Sidler (old account; do not CC)]

Mark?...

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

Comment 41

[Mark Mentovai]

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?

Comment 42

[Samuel Sidler (old account; do not CC)]

(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.

Comment 43

[Mark Mentovai]

(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.

Comment 44

[Mark Mentovai]

Attachment: Working 1.7 branch patch

(Tested)

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

Comment 45

[Simon Fraser [no longer active]]

Comment on attachment 214269 [details] [diff] [review]
Working 1.7 branch patch

That looks more like it.

Comment 46

[Mark Mentovai]

Checkered in to 1_7.

Comment 47

[Samuel Sidler (old account; do not CC)]

v all around.