Closed Bug 580230 Opened 14 years ago Closed 14 years ago

Cannot submit forms


(Firefox for Android Graveyard :: General, defect)

Not set



Tracking Status
fennec 2.0a1+ ---


(Reporter: ahoza, Assigned: jbos)



(1 file, 3 obsolete files)

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, enter location for a picture and select "Upload"
3. Open 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.
Whiteboard: formfill
tracking-fennec: --- → ?
It works for me each time but I thought to have seen someone else saying something similar on #mobile.
tracking-fennec: ? → 2.0a1+
I suspect regression caused by fix to bug
(In reply to comment #2)
> I suspect regression caused by fix to bug

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

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.
Attached patch Add pref check (obsolete) — Splinter Review
Added a pref check and tested this again to confirm that calling sendAsyncMessage() is causing the issue on non e10s build.
Ever confirmed: true
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
Attachment #459469 - Flags: review-
Assignee: nobody → 21
I created build environment on my desktop as same as on device and reproduced the issue on desktop also.

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

Additional changes applied to the sources:
* Applied xremote patch for mozilla-Qt from bug

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
6. Type "fennec" in YouTube's search field 
7. Tap on Youtube's search button

Actual outcome:
Nothing happens

Frequency of occurrence: 

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]
Ok i got a look into it and found a reproduceable cause.  its happening when you close the first tab.
Attached patch Remove SubmitObserver correctly (obsolete) — Splinter Review
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.
Attachment #460358 - Flags: review?(mark.finkle)
The invalid pointer Jeremias is referring to is here:

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.
Attachment #459469 - Attachment is obsolete: true
Comment on attachment 460358 [details] [diff] [review]
Remove SubmitObserver correctly

>+  receiveMessage: function findHandlerReceiveMessage(aMessage) {
>+    let json = aMessage.json;
>+    switch ( {
>+      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....
Attached patch Version 2 (obsolete) — Splinter Review
Updated the patch.
Attachment #460358 - Attachment is obsolete: true
Attachment #460555 - Flags: review?(mark.finkle)
Attachment #460358 - Flags: review?(mark.finkle)
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?
Names where changed to tapClose / tapOpen. and not using Detach/attach anymore.
Attachment #460555 - Attachment is obsolete: true
Attachment #460692 - Flags: review?(mark.finkle)
Attachment #460555 - Flags: review?(mark.finkle)
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.
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 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
Attachment #460692 - Flags: review?(mark.finkle) → review+
Whiteboard: formfill
Closed: 14 years ago
Resolution: --- → FIXED
Filed bug 582836 to clean up the messages in the future
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 is submitted.
2. Can log in on
You need to log in before you can comment on or make changes to this bug.