Status

VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: ahoza, Assigned: jbos)

Tracking

Trunk
x86
Linux

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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.
Whiteboard: formfill
tracking-fennec: --- → ?
It works for me each time but I thought to have seen someone else saying something similar on #mobile.

Updated

8 years ago
tracking-fennec: ? → 2.0a1+

Comment 2

8 years ago
I suspect regression caused by fix to bug https://bugzilla.mozilla.org/show_bug.cgi?id=574699
(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

8 years ago
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

8 years ago
Created attachment 459469 [details] [diff] [review]
Add pref check

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

Updated

8 years ago
Status: UNCONFIRMED → NEW
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

Comment 7

8 years ago
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]
(Assignee)

Comment 8

8 years ago
Ok i got a look into it and found a reproduceable cause.  its happening when you close the first tab.
(Assignee)

Comment 9

8 years ago
Created attachment 460358 [details] [diff] [review]
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.
Attachment #460358 - Flags: review?(mark.finkle)
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.
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 (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....
(Assignee)

Comment 12

8 years ago
Created attachment 460555 [details] [diff] [review]
Version 2

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?
https://developer.mozilla.org/en/nsIObserverService/addObserver
(Assignee)

Comment 14

8 years ago
Names where changed to tapClose / tapOpen. and not using Detach/attach anymore.
(Assignee)

Comment 15

8 years ago
Created attachment 460692 [details] [diff] [review]
Version 3 - Fixed Typo
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
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Filed bug 582836 to clean up the messages in the future
(Reporter)

Comment 21

8 years ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.