From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; m17) Gecko/20000807 BuildID: 2000080712 Browser doesn't wait for the security warning about submitting insecure data before proceeding. Reproducible: Always Steps to Reproduce: 1. Open Mozilla (fresh) 2. Go to http://www.mozilla.org/quality/help/bug-form.html 3. under search bugzilla type anything (for example, security warning) 4. Press enter (clicking with the mouse doesn't give you security warning, not sure if it is a bug though) 5. A warning box will pop up, saying are you sure you want to submit? Actual Results: The browser windows will pop up with the result even if you didn't click on security dialog yet. Expected Results: Wait for my input first, before proceeding with search. 1) There also should be a way to select whether you want that warning to pop up or not from settings. (I couldn't find it though). 2) There should be no difference between submitting with pressing enter or with the mouse, it should give the same warning.

Changing component to Crypto.

Reproducable.

The Bugzilla Helper isn't expecting the form to be submitted.

Last time I saw this behavior it was a problem with calling the nsIDOMWindow methods not blocking until the user had pushed a button.

Since this is nsbeta3+, setting priority to P2

PDT agrees P2 (lchiang temporarily using trudelle's system).

I'm gonna need help fixing this one. Here's a summary of what's going on: Going to the form described above properly calls the CheckPost method which prompts the user. The method nsSecureBrowserUIImpl::CheckPost properly calls dialog->ConfirmCheck but before the call to ConfirmCheck returns Mozilla has already opened up a new window with the search results. Seems the event to open a new window is queued up and processed. This doesn't a ppear to be a bug in psm-glue. Who else can look at this? Perhaps DOM or forms are the problem here.

Sounds like we need blocking dialogs. danm?

Stack trace looks to me as if the newly created window(s) have JS event handlers that are explicitly opening new windows. This doesn't make much sense to me, and of course JS is a huge black hole in a C++ stack trace. But in the attached trace, toward the bottom, you'll see nsSecureBrowserUIImpl::CheckPost. Half a dozen lines above that, the modal loop. Above that, a window (presumably the alert) has been loaded, fires a focus event, some other window catches the corresponding blur, some text field's JS event handler opens a new window(!?), and the onload handler of that window kind of does the same thing, opening a third window. I don't really get it, but it might be useful to try to figure out what JS is executing in that stack trace.

Something else that should be added to this bug is that 2 windows with the same content come up after hitting enter. There is something else going on here that is beyond my area of expertise. The longer I look into this bug, the more I'm convinced it should belong to some one else. Probably dom or some other component.

Adding comments sent to me by jst: Javier Delgadillo wrote: > Hello jst, Hello Javier! > I don't know if you're the right person for this bug, but since you > helped me last time I needed help with dom issues, I'm forwarding this > to you hoping you can at least refer me to the right engineer. > I might not be the right person for this problem, but read on... > I'm working on bug 48064 > (http://bugzilla.mozilla.org/show_bug.cgi?id=48064). >From my > debugging I've found the following: > > in forms, if you hit enter and have the preference for warning if > submitting data to a site insecurely mozilla doesn't wait for you to > hit OK or cancel before bringing up the new window (matter of fact, it > brings up two windows with the same content). > > My current theory is that in nsGfxTextControlFram2::SubmitAttempt a > call to HandleDOMEvent posts an event that doesn't wait on the user to > click OK or Cancel before it gets processed. Is this even close? > Should I be looking somewhere else? > I know there are various similar problems in mozilla right now, http://bugzilla.mozilla.org/show_bug.cgi?id=52667 is one of those, and I talked to Rick Potts about this and the problem seems to be that mozilla processess PLEvents in the wrong order when a modal dialog is open, IOW, this is a very low-level problem that needs to be fixed before you can continue working on what could possibly be the real problem in bug 48064. Also, there's a bug on pressing enter in an input field doesn't fire the onchange event handler, you'll notice that on the bugzilla page mentioned in the bug.

After extensive debugging, I've come to the conclusion that the code in psm-glue is doing the right thing, ie it prompts the user to see if he/she wants to continue to when submiting data to an insecure site. The bug lies somwhere in the dom code or the form handling code. Re-assigning to jst so he can re-assign to the engineer who is more qualified than me to handle this bug. (Sorry jst, I lost the e-mail where you told me who the main guy was for form submission.)

Marking nsbeta3-, putting on rtm radar, re-assigning to Eric Pollmann, the owner of the form submission code and the person that Javier originally wanted to re-assign this bug to. Eric, I haven't done an in depth analysis of the bug, so please give it to the right person if it isn't you. Thanks!

As Johnny mentioned above, this sounds like it could be a dup of bug 52667. I tried the fix attached there with mixed results: - The form no longer submits until Okay is pressed, but... - The form no longer submits at all. This looks like a nasty one... :S I'll take a closer look.

Eric, try out my patch that I posted on the xpcom newsgroup. It sounds like a dup of the nested event queue problem.

Okay, the more I look at this the less sure I am that these behaviours are bugs. First, here's the HTML for the text input on the page: <INPUT type=text size=60 name=keywords onChange="SearchForBugs(this.form.keywords.value,this.form.searchLong)"> When Enter is pressed in the text input, two things happen. First, the onclick event handler is fired. This calls SearchForBugs. In SearchForBugs, we never submit the form, we instead open up a new window named "other" with a URL that is manually generated in the function to look like a form post. You'll notice in Nav 4.x, the new window opens with the search results before you acknoledge that the for post is insecure too. Second, since the click event is never cancelled with a "return false" in the event handler, the form goes ahead and submits. This causes a warning dialog to pop up, but should not prevent the above page load (not the result of the form submit) from happening. However, once you click "ok" on the warning dialog, it should should cause the original page to reload with some extra cruft after the URL, like this: http://www.mozilla.org/quality/help/bugzilla-helper.html?keywords=security+warning&searchLong=min Okay, now let's look at the HTML for the button and see what that does: <INPUT type=button value=Search onClick="SearchForBugs(this.form.keywords.value,this.form.searchLong)"> Clicking on the button will have the same effect as pressing Enter in the text field. That is, a new window is opened up, and a manually generated URL is loaded into that window. Since this is not the result of a form submit, no warning dialog should appear. The difference here is that <input type=button> does not cause a form submit to appear when it is clicked on (<input type=submit> does that) That is why no warning dialog ever appears. The only behaviour that is wrong in this test case is that two windows are opened up. I'm looking into that...

When entering a search term, then clicking the "Search" button, we cause two things to happen: 1) The text field gets blurred - this causes an onchange to fire (per the HTML spec). This causes the onchange event handler to fire (assuming the text in the input has changed since it last received focus) 2) The button is clicked - this causes an onclick event to fire (again, per the spec) That explains the two windows in the button click case However, it looks like there is one bug - there should only be one window when you press Enter in the text input. However, the onchange event is firing twice. The first time it is firing, there is a nested event loop - and a OnEndLoad is firing on a crome channel (presumably the security warning dialog) in the first event loop, which causes the onblur to fire in the second event loop. I'll try Doug's fix to see if that fixes this problem...

I tried the patch again (fresh tip build), and had the same results as before. Instead of doing things mostly right like we are without the patch, now the security dialog pops up and just halts the browser in it's tracks. Nothing loads in the new windows. In fact, I see nothing load in the window created when clicking the "Search" button either.

Updating summary to reflect the only bug I found in our behaviour on this page.

Marking rtm+.

Marking [rtm need info] Waiting for patch to be attached, review + super-review.

I'll take this one and see what I can find

Trying the newest patch that Doug posted, fixed the loading problem, but we still post two onchange events. The double onChange event may be unrelated to the nested event loop changes Doug is working on.

This is an editor problem. The problem is when you hit the return key you get on onchange event and a submit. When the page is going to be disposed of because of the submit the focus is removed from the text field and the "blur" processing causes a second onchange event to be fired. So you only see the double onchange event when you have an onchange event handler on the test field instead of a form where a submit will take place. note: in the attached reduced testcase, if you remove the form so the submit doesn't happen you only get on onchange fired. reassigning to the editor group.

After speaking with kevin, he was wondering if the text field should be getting a blur when the document was going away. Something to think about anyway.

handing this over to Simon for review

Simon, shouldn't this rally go to joki? it seems like it is more in his area than it is an editor issue

-> Mike

i have been messing with this and i cant seem to see 2 blurs. i will look into this more.

ok i belive i have a fix i will post bug. this is LOW RISK I REPEAT LOW RISK. in fact i think it was an oversight on my part that i did not update mFocusedValue unless a focus was called and did not think that multiple blur/entrekey could be called until a focus was called. post rtm we should remove the need for a string such as focusstring and use the editors internal state to decide if something has changed.

Yup, looks similar to Windows-specific that code chouck wrote five years ago to implement onchange in the pre-beta-Nav2.0 WinFE. a=brendan@mozilla.org. Who will r=? /be /be

This looks like a dup of bug 51268. It seems to happen on Linux only, though. joki tried fixing that (he has no access to Linux at the moment) but the fix did not work. However, the fix seemed to be in the same direction as mjudge's. I am adding joki to CC, can you review? And if someone could try the attached testcase in bug 51268 and see if this patch fixes that bug as well...

Looking at the patch... Do we need to be defensive and check mFrame after each call to CallOnChange(), in case the onChange handler did something that blew away frmaes (see bug 51772)?

this is NOT a combination of the two patches. yes this will be taken into account when i merge the two changes.

setting to rtm+ to get attention i guess.

You have an "a" from brendan, but you still need a second reviewer r= Please get the reviews lined up, and then nominate to rtm-plus. Marking rtm need info. Also... subtle question.. is it really the correct order to first call the onchange() method, and then set the member to the new state? I'm not familiar with the code, but I would have guessed that the state would first be changed, and then the onchange() event would be queued (maybe all events get serviced on this solo thread... and it is no big deal... but the reverse order makes more sense to me... but I don't know how the code is structured). It would also be a shame if the onchange() synchronously (or rapidly) processed the event, based on the *old* state (gulp). Oh well.. just trying to help with the reviews. Jim

jar: it turns out not to matter. Everything is synchronous on the main thread, you can't nest KeyPress events, and mFocusedValue is used only (check lxr) to decide if the value on blur differs. mjudge is right that this code needs to be improved later, on the trunk, but I think he has a minimal fix for the branch. I agree that setting mFocusedValue before calling onchange is the more general order, if there were any way for the event handler to depend on mFocusedValue. To get an r= from sfraser and an a= from me on the combined patch, though, maybe we should see a patch that merges this one with the fix for bug 51772. mjudge, whaddya say? /be

ok here is the unified diff of 51772 and 48064. ready to go and works well. only 1 onchange fired when hit return and no crashing.

setting to m19

adding rtm + and sending off to pdt for approval, this also fixes 51772 +

marking 51772 as a dup of this since the fix for this one includes the fix for that one

*** Bug 51772 has been marked as a duplicate of this bug. ***

PDT says need a= and r= on the actual attached combined patch.

PDT would also like to know why the patches were combined. If bug 51772 is a dup of this, why was a separate fix for it merged into the fix for this?

patches were combined because I was TOLD to combine them. I dont care, I can uncombine them. please read 51772 it describes why i combined them. I dont want to bother people to get r= or a= if the bugs wont be taken anyway. I hate to waste peoples time. If all you need is r= and a= by someone you trust please let me know and i can get that in 10 min. thanks

crap gotta mark this rtm+ or it wont show up.

Please come discuss this in person at the PDT meeting Friday. We'd like to resolve this in one shot rather than drag it out over email and bugzilla comments.

rtm++ after talking to mjudge

See bug 57514 - there are two onchange events firing when pressing Enter in a text input on that test case as well.

As far as I can tell, mjudge's latest patch has been checked in. I see only one onChange event being fired after hitting return in a text field. Is this bug fixed then? If so, I have another complication to add, garnered from bug 57514. The test case in that bug issues an Alert in a text field's onChange handler. So after hitting <return>, in the middle of processing the onChange handler, the alert window comes up, the text field is blurred, and its onChange handler called a second time, resulting in a second alert. Note that Navigator 4 does nearly the same thing (two alerts, but not simultaneously) while IE brings up only one alert. Gosh, they're good. I notice that if I change mjudge's latest patch to set mFocusedValue = blurValue before calling CallOnChange in nsTextInputListener::KeyPress, only one alert is issued. Just a thought.

jar was right, I was wrong: our modal dialogs are synchronous, so update "changed" state before dispatching the event handler. How about doing a quick patch for that bug, mjudge? /be

jeesh i did it in one location of callonchange and not the other. moved the setting of mFocused value to pair up with callonchange not submitattempt. I was trying to be too clever in not wasting time when frame was going away. ok i feel rather embarassed but here is another simple patch here with comment explaining what i tweaked.

r=brendan@mozilla.org -- get an sr= and get this in the trunk, ok? Can we go for limbo-rtm? /be

sr=buster. you should probably carry over the "crash" keyword from bug 57514 since if I understand it right the way things are today we crash in that case, and your latest patch here fixes that. That will make it more likely PDT will still accept this fix.

Patch looks good, r=sfraser

This fix has missed the first N6 candidate build, so we can not take it unless we respin. This bug is in candidate limbo. We will reconsider this fix once we have a candidate in hand, but we can't take this fix before then. PDT marking [rtm+]

PDT marking [rtm++]. This bug is now out of limbo and approved for checkin to the branch. Please check in ASAP.

The fix has been checked in on the branch. The trunk has been in flames all afternoon, so we'll have to wait a little while to get the fix there. Hence, not yet marking this bug fixed.

This works on the Win, Mac and Linux 10/31 branch builds.

Akkana: Can we check this in to the trunk now? It's been almost a week. Also, can we rtm- this now to get it off the rtm radar?

Mike? Were you going to check this in on the trunk, or did you want me to?

Marking fixed, since this now works on the 11/13 trunk builds.

Verified.