169943 - Form submit buttons not working [embedding apps]

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Tracy Walker [:tracy]]

seen on embed builds:

mfcembed 2002-09-20-08-trunk
testgtkembed 2002-09-20-10-trunk
ppembed 2002-09-20-08-trunk

-goto www.google.com
-enter a search subject
-Click "Google Search"

expected results:  The form is submitted and search results are returned

tested results: The form submit button doesn't work.  nothing happens.

Comment 1

[Frank Tang]

how about mozilla app?

Comment 2

[Christopher Blizzard (:blizzard)]

Why is this assigned to me?

Comment 3

[bobj]

WFM on today's commercial trunk:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2b) Gecko/20020920
Netscape/7.0b1+

Comment 4

[Asa Dotzler [:asa]]

tracy, was this working yesterday?
->form submission.

Comment 5

[Tracy Walker [:tracy]]

Asa, I have been these embedding apps Mon. Wed. and Fri.  It was working fine on 
Wednesday.  

Yes, it works for me too on commercial builds today.  Just seeing this on the 
embedding apps.

Comment 6

[adrian]

Broken for me on a Cvs pull 2002-09-20-08-trunk 2:30am linux

linux only?

Comment 7

[Asa Dotzler [:asa]]

John, can you take a look at this as soon as possible. I'll start reading
through bonsai.

Comment 8

[Asa Dotzler [:asa]]

one more datapoint. This was broken in yesterday's winembed too. 

Comment 9

[Asa Dotzler [:asa]]

This worked on wednesday. doesn't work on thursday.

steps tested:
1 load google.com
2. enter search term "foo"
3. hit submit

expected results: with wednesday's build I get a security warning about
submitting data unencrypted and if I say OK then I get the google results. 

actual results: with thursday's (or today's) build I get no security warning and
no search results. 

Comment 10

[bobj]

Is this trunk only.  Do we know if 1.0 branch is OK?

Comment 11

[Jaime Rodriguez, Jr.]

this issue was only seen on the trunk builds fro yesterday and today correct
(i.e. not the 1.0 branch)?

Comment 12

[Akkana Peck]

I'm not seeing this in my linux debug builds today of either mozilla or
gtkembed.  I can type strings into google, hit return or click the Google Search
button, and I get the appropriate list of google hits.  Do we have any idea
what's special about the builds that are showing the problem?

Comment 13

[Asa Dotzler [:asa]]

looks like a packaging issue. it works in the embedding control that comes with
the regular mozilla zip builds, just not in the build packaged exclusively for
embedding. Not a smoketest blocker. 

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The only suspicious checkin seems like bug 166945, although I could be wrong.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I did an experiment in my own debug build -- I removed libpippki.so and
pippki.xpt, and tried a form submission in TestGtkEmbed, and it didn't work. 
Based on that, assigning to kaie.

Comment 16

[Mike Kaply [:mkaply]]

Just to confirm what dbaron said, same thing is true on Windows. Without
pippki.dll, clicking the button on google does nothing.

Comment 17

[Kai Engert [:KaiE:]]

The PKI package implements a couple of UI alerts that are shown to the user.
One of those alerts is nsISecurityWarningDialogs::confirmPostToInsecure().

nsSecureBrowserImpl::confirmPostToInsecure, which is still contained in the
embedding package, tries to obtain the implementation.

For safety reasons the logic being used is:
  "If there is no such implementation, assume the user can not
   be warned, and do not allow to continue the operation."

PKI implements the required contract ID and interface, the confirm method of the
implementation gets called and can bring up the UI.

But I'm a bit surprised. PKI implements the code using a XUL dialog. I thought
XUL dialogs would not work in embedding applications? Did the embedding
application ever show a confirmation dialog when submitting a form like Mozilla
does, or did it submit without a warning? Maybe the implementation was found at
runtime, but the implementation was unable to bring up the XUL dialog, and used
a different failure return code? This looks like the possible reason, since the
PKI implementation returns the "continue submit" flag on failure, while the
non-existence of the implementation causes a "cancel submit" flag to be returned.

That's the first part of the story which leads to the question: Do the embedding
packages implement nsISecurityWarningDialogs?

Well, but unfortuantely that's only the first part of the story.

Bug 168448 is also related, and both bug 168448 and bug 166945 landed within the
last few days.

Bug 168448 is the result of change requests from the embedding team. Instead of
using a single contract ID mapping to several interface implementations, we
removed the old contract ID and empty interface class (nssdialogs) and
introduced separate contract IDs for each interface class.

Summary:
- If you do not yet implement you own version of nsISecurityWarningDialogs and
its new contract ID, then you should. If you don't want to bring up warning
dialogs, then you at least need to return TRUE in those dialogs, to signal you
don't want to stop the submission.
- If you do implement nsISecurityWarningDialogs, make sure you are registering
it with the new contract ID.

Comment 18

[John Keiser (jkeiser)]

I think the logic itself is wrong.  If the application has no implementation it
obviously does not *want* the dialogs, and I think it is very very clear that
even an implementation without the dialogs would want to be able to submit forms ;)

The insecure warning in general is pretty annoying but that's a different bug.

Comment 19

[John Keiser (jkeiser)]

Attachment: Patch

Confirmed, kaie has the right of it--if you don't implement nsISecurityDialogs
you don't submit.  This patch should allow embedders to submit without
implementing nsISecurityDialogs, which is the right thing IMO--it would be a
rare embedder indeed that wanted to disable all submissions.  There are no
other uses of nsISecurityDialogs in that file that need to be fixed.

Comment 20

[John Keiser (jkeiser)]

(I tested this patch by forcing GetNSSDialogs() to return null: before the patch
submission failed, and after the patch it succeeded.)

Comment 21

[Kai Engert [:KaiE:]]

Comment on attachment 100075 [details] [diff] [review]
Patch

I suspect you accidentially attached a wrong patch? This looks unrelated.

Comment 22

[Kai Engert [:KaiE:]]

CC'ing Stephane and Javi in case they want to veto.

The existing implementation requires embeddors to implement the security warning
dialogs. If they do not, form submit will not work.

In general I think this requirement is a good idea. It forces embeddors to think
about the risk for user's at least once. If they don't care for the risk, it's
easy to provide a dummy implementation.

Comment 23

[Chak Nanga]

This used to work (i.e. got security warning dialogs and was able to submit)
with earlier MfcEmbed builds.

Here's how i tested:

1. Downloaded the 9/16 embed dist from
ftp://ftp.mozilla.org/pub/mozilla/nightly/2002-09-16-08-trunk/embed-win32.zip

When i ran MfcEmbed from this build i get a security dialog and then the google
from submission was successful.
[I will submit an image of the security warning i see as an attachment]

2. Downloaded the latest embed dist. I do not see the security dialogs.

The _native_ (non-XUL) security dialogs were being shown in MfcEmbed eventhough
it did not impl nsISecurityDialogs. I think these dialogs were being shown via
the nsIPromptService which MfcEmbed implements.

Comment 24

[Chak Nanga]

Attachment: Security Warning dialog which gets shown in MfcEmbed on form Submits.

This is what i see in the 9/16 Windows embed dist builds.

Comment 25

[Kai Engert [:KaiE:]]

Chak, in that case it probably worked, because in the past we accidentially
included Mozilla's UI package with the embedding package.

The question is now what you prefer:

Option 1)
We could move the implementation of nsISecurityWarningDialogs to those portions
of PSM that are distributed with the embedding package.
The UI would continue to use nsIPrompt, but not XUL.

Option 2)
No longer provide the default implementation, but require embeddors to implement.


Since the recent change, we currently have 2)

But as it worked in the past, it sounds reasonable to do 1) ?

Comment 26

[John Keiser (jkeiser)]

Attachment: Patch

This is the actual patch.  I have changed it to add an assertion when
nsISecurityDialogs is not present.

kaie, I can see the argument, but I think the current situation is bad
nonetheless--it's silent failure.  If we we really want to force embedders to
bring up these dialogs we should at least assert (maybe crash) and refer to a
new chapter to the embedding guide.  I favor asserting but allowing submission
by default so that the embedder will at least have something working. 
Embedding Gecko should be as easy as it can reasonably be.

Comment 27

[John Keiser (jkeiser)]

Chak, I do feel like kaie did the right thing removing the dialog for embedders:
the security dialog should be the embedder's business.  If we put in an actual
dialog it should be at least configurable via a build option.  [Rant about the
essential purposelessness of these dialogs deleted]

Comment 28

[Kai Engert [:KaiE:]]

John: However, even if Mozilla provides default dialogs, embeddors can still
provide an overriding implementation, doing whatever they prefer.

Comment 29

[Kai Engert [:KaiE:]]

Comment on attachment 100249 [details] [diff] [review]
Patch

I discussed with Chak and we want to go with Option 1.
I'll come up with a patch soon.

I also discussed with John and convinced him that adding back the
implementation
restores the previous behaviour. (I wouldn't have changed it if I had realized
this doesn't require XUL.)

I convinced John the decision, whether those dialogs should be on or off by
default is a separate bug/discussion.

Comment 30

[Kai Engert [:KaiE:]]

Attachment: Patch to move implementation

This patch moves the implementation.

Note to reviewers: Things are just being moved! No logic changed or added!
		   -------------------------------------------------------

Because of that, I'm using the existing license headers for the new files.

Comment 31

[Kai Engert [:KaiE:]]

Javi, can you please review?

This patch moves the security warnings implementation from package pippki (no
longer included in embedding) to package pipboot.

Comment 32

[Javier Delgadillo]

Comment on attachment 100269 [details] [diff] [review]
Patch to move implementation

I'd rather move the implementation so pipnss since pipboot was designed to be
the minimal implementation to prevent the loading of NSS.   So in order to keep
pipboot as small as possible, I prefer moving the implementation to pipnss.

If you feel strongly that pipboot is the better place, convince me of it.

Comment 33

[Javier Delgadillo]

Comment on attachment 100269 [details] [diff] [review]
Patch to move implementation

OK, kaie convinced me.	r=javi

Comment 34

[John Keiser (jkeiser)]

If you're moving it into pipboot, perhaps bug 154723 will be fixed as well.

Comment 35

[Steve Wardell]

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

Comment 36

[rpotts (gone)]

Comment on attachment 100269 [details] [diff] [review]
Patch to move implementation

sr=[e-mail]

Comment 37

[Kai Engert [:KaiE:]]

Fixed on trunk.

Comment 38

[Steve Wardell]

I see this problem in the main Mozilla builds trunk in 2002092404 on W2K. Is 
this the same issue and the patch is just not in this build? The steps to 
reproduce are the same.

Comment 39

[Tracy Walker [:tracy]]

this is also on chimera 2002-09-24-04

Comment 40

[Kai Engert [:KaiE:]]

I you still see this problem, please reopen the bug.

Comment 41

[Steve Wardell]

A delete and new extraction of the nightly fixed this problem for me using a
version after this fix was checked in. I don't think this was an embedding only
problem thou.

Comment 42

[Jaime Rodriguez, Jr.]

kai: just to be doubly sure, this does not need to be fixed on the 1.0 branch,
right?

Comment 43

[Kai Engert [:KaiE:]]

Jaime: No change for the 1.0 branch is required. This bug was a result of some
pre-interface-freeze-cleanup, that only landed on the recent trunk.

Comment 44

[Tracy Walker [:tracy]]

verified with embed builds:

windows mfcembed 2002-10-07-08-trunk
linux testgtkembed 2002-10-07-08-trunk
mac osx ppembed 2002-10-07-08-trunk