580230 - Cannot submit forms

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[ahoza]

User-Agent:       Mozilla/5.0 (Windows; Windows NT 6.1; en-US; rv:2.0b2pre) Gecko/20100718 Minefield/4.0b2pre
Build Identifier: Mozilla/5.0 (X11; Linux armv7l;en-US;rv:2.0b2pre) Gecko/20100720 Namoroka / 4.0b2pre Fennec/2.0a1pre

Forms are not submitted.

Reproducible: Always

Steps to Reproduce:
1. Start Fennec
2. Open imageshack.com, enter location for a picture and select "Upload"
3. Open http://www-archive.mozilla.org/wallet/samples/sample2.html. Enter information in "First Name" and "Last Name" fields and then submit 

Actual Results:  
Nothing happens when trying to submit forms.

Expected Results:  
After step 2 page is uploaded on server.
After step 3, form is submitted and fields are cleared.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

It works for me each time but I thought to have seen someone else saying something similar on #mobile.

Comment 2

[Sudarsana Nagineni]

I suspect regression caused by fix to bug https://bugzilla.mozilla.org/show_bug.cgi?id=574699

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #2)
> I suspect regression caused by fix to bug
> https://bugzilla.mozilla.org/show_bug.cgi?id=574699

Sudarsana, could you reproduce the bug? And if yes are you on device or desktop?

Also even if the name (FormSubmitObserver) is really close to "submit" button it should not have impact this feature. For my part I suppose that I have broke something during the refactoring of the Form Assistant code.

I will be really happy to fix this one if only I could reproduce it :(

Comment 4

[Sudarsana Nagineni]

I am able to reproduce the problem on my device. I am getting an uncaught exception error from the following line when the issue is reproducible.

http://mxr.mozilla.org/mobile-browser/source/chrome/content/content.js#822

I am running non e10s build and I don't have an idea why sendAsyncMessage() is getting called.

I also checked this issue on e10s build and it is not reproducible.

Comment 5

[Sudarsana Nagineni]

Attachment: Add pref check

Added a pref check and tested this again to confirm that calling sendAsyncMessage() is causing the issue on non e10s build.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 459469 [details] [diff] [review]
Add pref check

We shouldn't need to do this. The code should work the same in both cases. Let's find the real problem.

I am tempted to mark this _not_ blocking 2.0a1 since by default we ship Fennec with browser.tabs.remote=true

But let's see if we can fix this quickly

Comment 7

[Sudarsana Nagineni]

I created build environment on my desktop as same as on device and reproduced the issue on desktop also.

Details are as follows: 
Sources:
* xulrunner: 06a37463c15e
* Mobile: 52f539338319

Additional changes applied to the sources:
* Applied xremote patch for mozilla-Qt from bug
https://bugzilla.mozilla.org/show_bug.cgi?id=184613

Build options:
* Toolkit: ac_add_options --enable-default-toolkit=cairo-qt
* Faststart module enabled: ac_add_options --enable-faststart

Pref change before starting fennec:
* browser.tabs.remote=false

Steps to reproduce the issue:
1. Start fennec:
   ./fennec &
2. Close fennec by clicking the X
3. This fennec process will stay in memory for X minutes because of faststart, you must do the next steps before that time expires:
4. Start fennec again using mozilla-xremote-client:
./mozilla-xremote-client -a fennec "OPEN()"
5. Open youtube.com
6. Type "fennec" in YouTube's search field 
7. Tap on Youtube's search button

Actual outcome:
Nothing happens

Frequency of occurrence: 
Always

Here is the error message I am getting in the terminal when I tap on the search button:

JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIFrameMessageManager.sendAsyncMessage]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: chrome://browser/content/content.js :: notify :: line 825"  data: no]

Comment 8

[Jeremias Bosch (:jbos)]

Ok i got a look into it and found a reproduceable cause.  its happening when you close the first tab.

Comment 9

[Jeremias Bosch (:jbos)]

Attachment: Remove SubmitObserver correctly

The issue we saw here was that in case a tab was closed, the submit observer wasn't removed - which caused a notify on a invalid pointer.

This fix removes the observer correctly when a tab closes. I tried different ways to notify the SubmitObserver that the tab was created or closed.

  1) "load", "unload" -Events: Those where never called.
  2) "pageshow", "pagehide" -Events: PageHide where not called on closing a tab, but when the user loaded a new link. PageShow gets called on opening a new Tab, but also when going to a new Website (by link,...). So it didn't fixed the initial issue of left over observers.

The only working attempt was to send a message from chrome to content after a new tab was opened or a existing one closed.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

The invalid pointer Jeremias is referring to is here:
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLFormElement.cpp#927

A simple enumerator is used to walk the list of "formsubmit" observers. A observer is adding for each tab. When a tab is closed, the observer was not removed, leading to the issue.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 460358 [details] [diff] [review]
Remove SubmitObserver correctly


>+  receiveMessage: function findHandlerReceiveMessage(aMessage) {
>+    let json = aMessage.json;
>+    switch (aMessage.name) {
>+      case "Browser:FormSubmit:Attach":
>+            Services.obs.addObserver(this, "formsubmit", false);
>+            break;
>+      case "Browser:FormSubmit:Dettach":
>+            Services.obs.removeObserver(this, "formsubmit", false);
>+            break;
>+    }

Only indent 2 spaces for the case blocks

>+    //do not nofify all tabs
>+    if (aWindow == content)

// Do not notify unless this is the window where the submit occurred

Overall, this patch works fine. I am little worried about needing to send TabOpen and TabClose messages into the child process.

If we do land this patch, I think we should change:
"Browser:FormSubmit:Attach" -> "Browser:TabOpen"
"Browser:FormSubmit:Detach" -> "Browser:TabClose"

just because the message is mirroring an event

Let me think a bit longer....

Comment 12

[Jeremias Bosch (:jbos)]

Attachment: Version 2

Updated the patch.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 460358 [details] [diff] [review]
Remove SubmitObserver correctly

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>--- a/chrome/content/browser.js
>+++ b/chrome/content/browser.js
>@@ -779,16 +780,17 @@ var Browser = {
>     this._tabs.forEach(function(aTab, aIndex, aArray) {
>       if (aTab.owner == tab)
>         aTab.owner = null;
>     });
> 
>     let event = document.createEvent("Events");
>     event.initEvent("TabClose", true, false);
>     tab.chromeTab.dispatchEvent(event);
>+    tab.browser.messageManager.sendAsyncMessage("Browser:FormSubmit:Dettach");
> 

>diff --git a/chrome/content/content.js b/chrome/content/content.js
>--- a/chrome/content/content.js
>+++ b/chrome/content/content.js
>@@ -807,23 +807,39 @@ var ContextHandler = {
>     sendAsyncMessage("Browser:ContextMenu", state);
>   }
> };
> 
> ContextHandler.init();
> 
> 
> var FormSubmitObserver = {
>-  init: function init() {
>-    Services.obs.addObserver(this, "formsubmit", false);
>+  
>+  init: function init(){
>+    addMessageListener("Browser:FormSubmit:Attach", this);
>+    addMessageListener("Browser:FormSubmit:Detach", this);
>+  },
>+

How does it work since it's listening "Browser:FormSubmit:DeTach" and the message send is ""Browser:FormSubmit:DeTTach""?
I assume it is a typo from code refactoring.

Also is it possible to use a weak observer here instead of doing the messages exchange?
https://developer.mozilla.org/en/nsIObserverService/addObserver

Comment 14

[Jeremias Bosch (:jbos)]

Names where changed to tapClose / tapOpen. and not using Detach/attach anymore.

Comment 15

[Jeremias Bosch (:jbos)]

Attachment: Version 3 - Fixed Typo

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 460692 [details] [diff] [review]
Version 3 - Fixed Typo

This patch looks fine, but I am checking to see if we can get a real "event" that is fired inside the child script before the tab is closed. That way we would not need to send any events from chrome.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

filed bug 582569 for firing an event when the TabChild closes, so we can remove the need for sending "Browser:TabOpen" and "Browser:TabClose"

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 460692 [details] [diff] [review]
Version 3 - Fixed Typo

We can take this for alpha 1 and remove the messages in a folloup bug

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/da65cd02aa1b

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Filed bug 582836 to clean up the messages in the future

Comment 21

[ahoza]

Verified as fixed with Nokia N900, buildID: Mozilla/5.0 (X11; Linux armv7l; rv:2.0b5pre) Gecko/20100823 Firefox/4.0b4pre Fennec/2.0a1 

Tested the following
 
1. Form http://www-archive.mozilla.org/wallet/samples/sample2.html is submitted.
2. Can log in on gmail.com