552828 - update Form History to work with Electrolysis

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Doug Turner (:dougt)]

Investigate how the autocomplete form feature will work in e10s

Comment 1

[Justin Dolske [:Dolske]]

Sounds like the plan here will be to:

1) have a piece living in content that implements (just) the nsIFormSubmitObserver
2) that piece will build up a list of data to be saved
3) use smaug's new message manager stuff (bug 549682?) to transfer the list from content to chrome
4) implement chrome receiver that loops over the list to call AddEntry on each

I'm not sure exactly how #1 gets implemented, but 2-4 should be able to land in mozilla-central (with the message passing simply going chrome-to-chrome, no IPC involved).

Another possibility is to remote the AddEntry call itself, but I'd be mildly concerned about performance. Seems cleaner to do it this way, too, since we know that the nsIFormHistory interface will change at some point.

Comment 2

[Benjamin Stover (:stechz)]

Note that for Fennec, we don't need to do a whole lot.  We can do autocomplete searching from the parent process, emulating an element with a JS object that has the "maxLength" property.

Saving is the only thing Fennec e10s needs to fix.  Lucky for us, autocomplete is being rewritten in JS (bug 439716).

(In reply to comment #1)
> Sounds like the plan here will be to:
> 
> 1) have a piece living in content that implements (just) the
> nsIFormSubmitObserver

We will need a small frame script that runs in content.  We'll use messageManager.loadFrameScript.

> 3) use smaug's new message manager stuff (bug 549682?) to transfer the list
> from content to chrome

That's the bug for putting the message manager in mozilla central for non-e10s.  Hopefully we will get it ported soon to e10s as well.  Otherwise, we may have to add a little code to detect what process we are in (with a TODO to change it later).

> 4) implement chrome receiver that loops over the list to call AddEntry on each

This could even live in FormHistory.

> Another possibility is to remote the AddEntry call itself, but I'd be mildly
> concerned about performance. Seems cleaner to do it this way, too, since we
> know that the nsIFormHistory interface will change at some point.

It should be trivial to send over arrays.  Message manager can work with JSON.

Comment 3

[Benjamin Stover (:stechz)]

Attachment: v1

First crack with updated tests that listen for the satchel observer events.  I think I at least need some license headers for the new files.

Comment 4

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

Comment on attachment 453235 [details] [diff] [review]
v1

Just a drive by. Patch looks good to me, based on what I know about using messagemanager and splitting chrome and content parts.

Comment 5

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

tested in fennec. autocomplete form fill works in both local and remote pages.

nice work!

Comment 6

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

Attachment: patch for fennec

Fennec patch needed to initialize the FormHitory component. Tested with local and remote pages.

Comment 7

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

Comment on attachment 453235 [details] [diff] [review]
v1

FormHistory should add "Ci.nsIFrameMessageListener" to its QueryInterface impl.

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 453235 [details] [diff] [review]
v1

>--- a/toolkit/components/satchel/Makefile.in
...
>+DIRS = public src content

Content-process stuff shouldn't live in /content, since that's normally only used for UI stuff (eg CSS/XUL).

Does the scriptloader need to live in a chrome:///content namespace due to permissions? If so, putting it there via jar.mn would be better.


>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/satchel/content/Makefile.in
...
>+# The Original Code is the Mozilla Password Manager code.
>+#
>+# The Initial Developer of the Original Code is
>+# Brian Ryner.
>+# Portions created by the Initial Developer are Copyright (C) 2003
>+# the Initial Developer. All Rights Reserved.

Update this stuff when you copy an existing file as a template. :)


>+++ b/toolkit/components/satchel/content/satchel.js
>@@ -0,0 +1,150 @@
>+dump("!! satchel.js loaded\n");

No dump, and this needs a MPL license header.


>+function SatchelFormListener() {
>+    Services.obs.addObserver(this, "earlyformsubmit", false);
>+
>+    this.prefBranch = Services.prefs.getBranch("browser.formfill.");
>+    this.prefBranch.QueryInterface(Ci.nsIPrefBranch2);
>+    this.prefBranch.addObserver("", this, true);
>+    this.updatePrefs();
>+}

Since there's only one instance of SatchelFormListener created, there's not much point in making it a prototype + |new SFL()|.

Just do var SFL = { ... }, move the constructor into an init() method, and call SFL.init() instead of the |new|.

>+SatchelFormListener.prototype = {
>+    QueryInterface : XPCOMUtils.generateQI([Ci.nsIFormSubmitObserver, Ci.nsISupportsWeakReference]),

Should have nsIObserver here too.

>+    prefBranch     : null,

Note to self: it's dumb we save the prefBranch, should just use Services in updatePrefs(). Fine as in, since you're just moving code. I'll fix in a followup.

>+    messageManager : null,

messageManager isn't set or used in this file, remove.

>+    log : function (message) {
>+        if (!this.debug)
>+            return;
>+        dump("FormHistory: " + message + "\n");
>+        Services.console.logStringMessage("FormHistory: " + message);
>+    },

Make these say SatchelFormListener instead.


>+++ b/toolkit/components/satchel/src/nsFormHistory.js
...
>+        this.messageManager = Cc["@mozilla.org/globalmessagemanager;1"]
>+                              .getService(Ci.nsIChromeFrameMessageManager);

We should have Services.jsm entries for this, I've got a patch to file.

>+        this.messageManager.addMessageListener("Satchel:AddEntries", this);

Let's do "FormHistory:FormSubmitEntries" instead. Hate to bikeshed, but I also hate the obscureness of the name "Satchel", and this avoids ambiguity with the AddEntries API (which this isn't directly related to).

>-        Services.obs.addObserver(this, "earlyformsubmit", false);

The QI impl in this file no longer needs nsIFormSubmitObserver.



>+    receiveMessage: function receiveMessage(message) {
>+        let entries = message.json;
>+        for (let i = 0; i < entries.length; i++) {
>+            this.addEntry(entries[i].name, entries[i].value);
>+        }
>+    },

Hmm, if message.json isn't really a json string, seems like we should change it's name. It's confusing to see this without having something parse the json...

Also, the for-loop needs to be wrapped in a transaction, as the old notify() method had.

>         } catch (e) {
>             this.log("getExistingEntryID failed: " + e);
>             throw e;
>         } finally {
>-            stmt.reset();
>+            if (stmt)
>+                stmt.reset();
>         }

What's this change for?


>+var checkObserver = {
>+  verifyStack: [],
>+
>+  observe: function(subject, topic, data) {
>+    netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+
>+    if (data != "addEntry" && data != "modifyEntry")
>+      return;
>+    ok(this.verifyStack.length > 0, "checking if saved form data was expected");
>+
>+    var expected = this.verifyStack.shift();
>+    ok(fh.entryExists(expected.name, expected.value), expected.message);
>+    finishPart();
>+  }
>+};

Hmm, I'm not terribly keen on having the test doing it's work asynchronously... Not having the submitted form and expected results being checked lock-step means that failures will look odd ("submitting test 10, oops test 5 failed"), and I'd be wary of introducing even nondeterminism into the tests.

I'd think it preferable to:
  1) submit a test form X
  2) wait for satchel-storage-changed notification
  3) check expected result
  4) X++
  5) lather, rinse, repeat

This might end up simpler too, since it's mostly just moving the "click next button" code into the observer, and having global for what new entry to check existence for.

Comment 9

[Philip Chee]

>+    log : function (message) {
>+        if (!this.debug)
>+            return;
>+        dump("FormHistory: " + message + "\n");
>+        Services.console.logStringMessage("FormHistory: " + message);

Sorry for the bug spam but why do we need both dump() and .logStringMessage()? Could it be made configurable e.g.
if (this.debugDump)
 dump()
else if (this.debugConsole)
 Services.console.etc..

Comment 10

[Benjamin Stover (:stechz)]

> Hmm, if message.json isn't really a json string, seems like we should change
> it's name. It's confusing to see this without having something parse the
> json...

message.json is the object that was transmitted over via JSON serialization.  I agree that there is ambiguity, but I don't really have a better name (message.object?).

Anyways, this is part of messageManager.  File a bug under IPC if you think this should be changed, but there's already a lot of code that depends on this name.

Comment 11

[Benjamin Stover (:stechz)]

Attachment: Review changes

Code review changes, except...

> Hmm, I'm not terribly keen on having the test doing it's work asynchronously...
> Not having the submitted form and expected results being checked lock-step
> means that failures will look odd ("submitting test 10, oops test 5 failed"),
> and I'd be wary of introducing even nondeterminism into the tests.

This would be a lot of added callbacks for these tests (especially test_form_submission.html).  I think you have an excellent point; maybe we could rewrite the tests using frequent yields?  Would it be possible to push this work to another bug?

Comment 12

[Benjamin Stover (:stechz)]

Sorry for bugspam, but I forgot to address one comment:

> >         } catch (e) {
> >             this.log("getExistingEntryID failed: " + e);
> >             throw e;
> >         } finally {
> >-            stmt.reset();
> >+            if (stmt)
> >+                stmt.reset();
> >         }
> 
> What's this change for?

dbCreateStatement managed to return null or throw once for me and I saw an error message, so I added the check.

Comment 13

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

> +        this.messageManager.loadFrameScript("chrome://satchel/content/child.js", true);

I wonder if we really want a new satchel.jar. or can we put the content script in toolkit.jar

If we move it to toolkit.jar, we should rename it so we don't collide (child.js is kinda generic). Maybe satchel-content.js ? I know we don't like "satchel" too

> +const satchelFormListener = {

Why go to lowercase?

Comment 14

[Benjamin Stover (:stechz)]

> Why go to lowercase?

I try to follow Crockford's advice of only naming constructors with capital letters, so there's an easy way to tell if calling a function should have a "new" in front of it.  This is consistent with the way messageManager is named.

Comment 15

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

messageManager is a property of an object, not a global.

Comment 16

[Justin Dolske [:Dolske]]

(In reply to comment #9)
> >+    log : function (message) {
> >+        if (!this.debug)
> >+            return;
> >+        dump("FormHistory: " + message + "\n");
> >+        Services.console.logStringMessage("FormHistory: " + message);
> 
> Sorry for the bug spam but why do we need both dump() and .logStringMessage()?

Because logStringMessage doesn't show up on stdout, so if you're watching the terminal you don't see these amongst other debug output.


(In reply to comment #12)
> > >         } catch (e) {
> > >             this.log("getExistingEntryID failed: " + e);
> > >             throw e;
> > >         } finally {
> > >-            stmt.reset();
> > >+            if (stmt)
> > >+                stmt.reset();
> > >         }
> > 
> > What's this change for?
> 
> dbCreateStatement managed to return null or throw once for me and I saw an
> error message, so I added the check.

Oh, yuck. Callers get the "stmt is null" exception instead of the exception we're trying to throw. Sadfaces.

This pattern is all over the current code, though, not just this one spot. Remove this change from your patch, and we'll fix them all in one go in a followup (filed bug 580839)


(In reply to comment #13)
> > +const satchelFormListener = {
> 
> Why go to lowercase?

Ben's explanation wfm, though we haven't been super-consistent either way over in /browser.

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 458457 [details] [diff] [review]
Review changes

>+++ b/toolkit/components/satchel/src/child.js
>@@ -0,0 +1,152 @@
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;

Still need an MPL header for this file.


>+    updatePrefs : function () {
>+        let prefBranch = Services.prefs.getBranch("browser.formfill.");
>+        prefBranch.QueryInterface(Ci.nsIPrefBranch2);

Don't need the QI here, that's only needed when adding an prefchange observer.


>+        if (entries.length) {
>+            sendAsyncMessage("FormHistory:FormSubmitEntries", entries);
>+        }

No need for braces here.


>+++ b/toolkit/components/satchel/src/nsFormHistory.js
...
>+        this.messageManager = Cc["@mozilla.org/globalmessagemanager;1"]
>+                              .getService(Ci.nsIChromeFrameMessageManager);

Huge mistake: the "." goes on the same line as the Cc[] for getService calls like these. :)


>+        this.messageManager.loadFrameScript("chrome://satchel/content/child.js", true);

I'll agree with mfinkle here, that "child.js" is a little too generic. formSubmitListner.js?


>+    receiveMessage: function receiveMessage(message) {
>+        let entries = message.json;
>+        for (let i = 0; i < entries.length; i++) {
>+            this.addEntry(entries[i].name, entries[i].value);
>+        }
>+    },

Still need the for-loop to be wrapped in a transaction, as the old notify()
method had... Otherwise all the separate DB hits can be really slow. (bug 426991)


>     updatePrefs : function () {
>-        this.debug          = this.prefBranch.getBoolPref("debug");
>-        this.enabled        = this.prefBranch.getBoolPref("enable");
>-        this.saveHttpsForms = this.prefBranch.getBoolPref("saveHttpsForms");
>+        let prefBranch = Services.prefs.getBranch("browser.formfill.");
>+        prefBranch = prefBranch.QueryInterface(Ci.nsIPrefBranch2);

As above, don't need the QI here.


r- for the above nits, but we're close!

Comment 18

[Justin Dolske [:Dolske]]

BTW, filed bug 580850 for getting message manager added to Services.jsm. Race to checkin! :)

Comment 19

[Justin Dolske [:Dolske]]

Attachment: Suggested test changes

(In reply to comment #11)

> > Hmm, I'm not terribly keen on having the test doing it's work asynchronously...
> 
> This would be a lot of added callbacks for these tests (especially
> test_form_submission.html).  I think you have an excellent point; maybe we
> could rewrite the tests using frequent yields?  Would it be possible to push
> this work to another bug?

I think it should be simple enough to fix here, here's a mostly-working (ie, broken :) patch that does what I'm suggesting.

Comment 20

[Benjamin Stover (:stechz)]

Attachment: Better tests and review changes

Does this look about right?  I went ahead and stopped copy-pasting checkObserver and put it in the common test file.

Comment 21

[Benjamin Stover (:stechz)]

Attachment: Fix fubar

Accidentally used old child.js

Comment 22

[Benjamin Stover (:stechz)]

Attachment: Fix fubar 2

Sorry for bugspam, but somehow had file as toolkit/components/src/formSubmitListener.js.

Comment 23

[Justin Dolske [:Dolske]]

Comment on attachment 461639 [details] [diff] [review]
Fix fubar 2

Almost, but not quite there! :)

>+++ b/toolkit/components/satchel/src/formSubmitListener.js
...
>+    notify : function(form, domWin, actionURI, cancelSubmit) {
>+        if (domWin.top != content.window)
>+            return;

Why was this check added?


Also, please wrap the entire contents of this function in a try/catch to suppress any errors. Better still, do what login manager does and just rename the guts to onFormSubmit(), and add a tiny notify() stub that calls it, ala:

  notify : function (...) {
    try {
      this.onFormSubmit(...)
    } catch (e) {
      // Suppress any errors, so failures don't cancel the form submission
      log("ah crap");
    }
  }

Sorry, should have caught (ha) that before; failures here cause nsHTMLFormElement.cpp to cancel the form submission, and that's really not what we want to have happen if satchel is busted somehow.


>+++ b/toolkit/components/satchel/src/nsFormHistory.js

>+        this.messageManager = Cc["@mozilla.org/globalmessagemanager;1"]
>+                              .getService(Ci.nsIChromeFrameMessageManager);

Style nit: while I'm here, the "." in ".getService" should go on the previous line.


>+++ b/toolkit/components/satchel/test/satchel_common.js

>+  observe: function(subject, topic, data) {
...
>+    var expected = this.verifyStack.shift();
>+    ok(fh.entryExists(expected.name, expected.value), expected.message);

Worth a comment here just to clarify that we're relying on satchel to deliver the correct number of add/modify messages, but we ignore what entry the message is for (and handle receiving too many messages as a failure, and too few will cause a timeout).

>+++ b/toolkit/components/satchel/test/test_form_submission.html
...
>     ok(true, "submitting subtest1");
>     iframe = document.getElementById("iframe2");
>     iframe.contentDocument.forms[0].submit();
>-    ok(!fh.entryExists("subtest1", "subtestValue"), "checking unsaved subtest value");

Hmm. This removes the test that's checking to see if the "don't save HTTPS formdata" is working.

Probably the easiest thing to to spin this subtest (and the other, for consistency) into their own not-subtests, a #21 that doesn't save and a #100 that does. The test doesn't currently get any events from the submission in the iframes, I _think_ <iframe onsubmit=...> will work in order to hook that up?


>+++ b/toolkit/components/satchel/test/test_form_submission_cap.html
...
>-  // make sure that the remaining changed fields are not saved
>-  ok(!fh.entryExists("test" + numInputFields, numInputFields), "checking unsaved value " + numInputFields);

Need to retain this check somehow, since it's the only think checking that those fields were not saved. Maybe add a checkForNotSave(), which pushes values onto a stack and ensures they don't exist once the normal checkForSave() stack is emptied?

Oh, but I guess this is already tested indirectly, because if these values are saved there will be extra satchel-storage-changed notifications, and those should cause a failure.

So, just add a comment here noting that this is what happens.



>+++ b/toolkit/components/satchel/test/test_form_submission_cap2.html
...
> var numSubmittedForms = 0;
> var numInputFields = 101;
> 
>+var numSubmittedForms = 0;
>+var numInputFields = 101;

Uhh, look 2 lines up? O_o.

Comment 24

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

(In reply to comment #23)
> Comment on attachment 461639 [details] [diff] [review]

> >+    notify : function(form, domWin, actionURI, cancelSubmit) {
> >+        if (domWin.top != content.window)
> >+            return;
> 
> Why was this check added?

if this check is needed, shouldn't it be:

  if (domWin.top != content)

"content" _is_ the top level nsIDOMWindow

Comment 25

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

Oh, and is the domWin always the "top" window? I forget what the C++ sends

Comment 26

[Benjamin Stover (:stechz)]

> Probably the easiest thing to to spin this subtest (and the other, for
> consistency) into their own not-subtests, a #21 that doesn't save and a #100
> that does. The test doesn't currently get any events from the submission in the
> iframes, I _think_ <iframe onsubmit=...> will work in order to hook that up?

I tried this.  I combined the iframe tests into one iframe, added a form in it, and tried button.click for the button in the iframe.  The onsubmit handler is not being fired though.

Comment 27

[Benjamin Stover (:stechz)]

Attachment: Patch

That was a toughie!  Found the issue.  display: none iframes have no pres shell and so clicking submit buttons does not force a form submit!  See:

https://bugzilla.mozilla.org/show_bug.cgi?id=584188

Patch with (hopefully) all review comments addressed.

Comment 28

[Benjamin Stover (:stechz)]

Attachment: Patch with observer fixes

This removes the check observer when test is done, so that XPConnect stops giving me errors.  What I'm seeing on try:

6317 INFO TEST-PASS |
/tests/toolkit/components/satchel/test/test_form_submission.html | form 20
submitted
6318 INFO TEST-PASS |
/tests/toolkit/components/satchel/test/test_form_submission.html | checking for
empty storage
6319 ERROR TEST-UNEXPECTED-FAIL |
/tests/toolkit/components/satchel/test/test_form_submission.html | checking if
saved form data was expected
6320 INFO TEST-PASS |
/tests/toolkit/components/satchel/test/test_form_submission.html | submitting
iframe test 21
6321 INFO TEST-PASS |
/tests/toolkit/components/satchel/test/test_form_submission.html | form 21
submitted
6322 ERROR TEST-UNEXPECTED-FAIL |
/tests/toolkit/components/satchel/test/test_form_submission.html | checking for
empty storage
6323 ERROR TEST-UNEXPECTED-FAIL |
/tests/toolkit/components/satchel/test/test_form_submission.html | checking if
saved form data was expected

(http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1281037785.1281038501.3280.gz)

I'm not having much luck figuring this one out.  It looks like the preference isn't getting set, but since this is all in the same process I am talking to the exact same preference service.  I'm not seeing this error on my machine.

Comment 29

[Justin Dolske [:Dolske]]

Comment on attachment 463701 [details] [diff] [review]
Patch with observer fixes

>+    notify : function(form, domWin, actionURI, cancelSubmit) {
>+        try {
>+            // Even though the global context is for a specific browser, we
>+            // can receive observer events from other tabs! Ensure this event
>+            // is about our content. 
>+            if (domWin.top != content)
>+                return;

Err, how does that happen? Is there a process per tab? (I thought it was just 1 content process for everything?)

Other than explaining this and the unexplained test failure, patch looks fine. \o/


(In reply to comment #26)
> I _think_ <iframe onsubmit=...> will work in order to hook that up?
> 
> The onsubmit handler is not being fired though.

Oh, duh, at the very least same-origin would prevent this. Ok.


(In reply to comment #28)

> 6319 ERROR TEST-UNEXPECTED-FAIL
> 
> I'm not having much luck figuring this one out.  It looks like the preference
> isn't getting set, but since this is all in the same process I am talking to
> the exact same preference service.  I'm not seeing this error on my machine.

Hmm, indeed. The other possibility being that you're getting an notification before you're expecting it, but that seems impossible because the log message indicates the next form hasn't even been submitted yet.

I can reproduce this on my local box, sometimes, so I'll investigate.

Comment 30

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

(In reply to comment #29)
> Comment on attachment 463701 [details] [diff] [review]
> Patch with observer fixes
> 
> >+    notify : function(form, domWin, actionURI, cancelSubmit) {
> >+        try {
> >+            // Even though the global context is for a specific browser, we
> >+            // can receive observer events from other tabs! Ensure this event
> >+            // is about our content. 
> >+            if (domWin.top != content)
> >+                return;
> 
> Err, how does that happen? Is there a process per tab? (I thought it was just 1
> content process for everything?)

There is one content process for every tab, but each tab will register an observer for a form submit. The C++ code, in the content process, enumerates over all of the observers, calling each one.

Comment 31

[Justin Dolske [:Dolske]]

Yeah, for some reason the sometimes neither of the pref observers are being called. When I add logging there, I see the output for green runs, but not for failures.

It's odd that just these 3 failures happen, because there are pref changes elsewhere in test_form_submission.html -- they're not logged either, but the tests are not failing. Hope they're not broken. :-o

Comment 32

[Justin Dolske [:Dolske]]

(In reply to comment #30)

> There is one content process for every tab, but each tab will register an
> observer for a form submit. The C++ code, in the content process, enumerates
> over all of the observers, calling each one.

So the script specified in messageManager.loadFrameScript() is being loaded for each tab?

Comment 33

[Justin Dolske [:Dolske]]

I think what's happening here is that the pref observer is being added with a weak reference (last arg to addObserver), and GC is somehow causing it to be removed.

For example, I can open a new tab, toggle browser.formfill.enable, and see the observer called (via logging output). If I wait a bit and toggle the pref again, the observer isn't called.

The odd thing is that the earlyformsubmit observer (which is added with a strong reference) is still being called, so the satchelFormListener object is obviously (?) still alive.

I wonder if this is some oddity with how loadFrameScript() loads and kills scripts? Or maybe just a bug somewhere in nsPrefBranch's code for adding and notifying observers? I'd suggest starting to debug there, and see what that code thinks is happening.

[I also wonder if we're gathering runtime bloat from all of formSubmitListener.js strong-ref addObserver() calls, since it never calls removeObserver. It's not leaking, and observers from closed tabs are not being called, but is this all just being suppressed somewhere internally?]

[[And I 3x wonder why there are 3 active form submit observers being called, when I've only got 1 window with 1 tab open. What are the other 2 bound to?]]

Comment 34

[Justin Dolske [:Dolske]]

Comment on attachment 463701 [details] [diff] [review]
Patch with observer fixes

Clearing review until the observer issue is understood, though I think this will be be fine to land as-is assuming the problem isn't in satchel code.

Comment 35

[Josh Matthews [:jdm]]

I suspect the mystery observers come from the incorrect loadFrameScript behaviour described in bug 584864.

Comment 36

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

Justin's brain dump caused me to remember a bug we fixed in the Fennec front-end code related to "formsubmit" observers. The C++ code does not honor weak references. You have to remove the observer manually before a tab is closed.

Yes, the loadFrameScript script is loaded for each new tab. But there is currently no trigger for when a tab is closed, so the script can't perform any proper cleanup. See bug 580230.

We added a stop-gap in that bug, but we are waiting for a "close" DOM event (bug 582569) so the script can perform real cleanup - like removing the "formsubmit" observer in this case.

Bug 584864 is also a good candidate, but I think the same thing that happened in bug 580230 is happening now. The observer is dead, killed when the tab closed, but no one ever told the C++ code so it tries to call it when enumerating "formsubmit" observers.

Comment 37

[Benjamin Stover (:stechz)]

> Oh, duh, at the very least same-origin would prevent this. Ok.

That's why we have enablePrivilege :) In case you didn't check out the tests, the subtests have indeed been split out into regular tests.  I worked around the presshell bug by making sure the iframe is rendered.

Comment 38

[Benjamin Stover (:stechz)]

> The odd thing is that the earlyformsubmit observer (which is added with a
> strong reference) is still being called, so the satchelFormListener object is
> obviously (?) still alive.

Yes, this is weird.  I am seeing the same thing.  Maybe the wrapper object is being garbage collected?  Could that happen?

Comment 39

[Benjamin Stover (:stechz)]

I've also verified that the pref listener in nsFormHistory.cpp also goes away.

Comment 40

[Benjamin Stover (:stechz)]

This is a known bug, apparently.  See:
https://bugzilla.mozilla.org/show_bug.cgi?id=533290

Comment 41

[Benjamin Stover (:stechz)]

After talking to Dolske on IRC, he thinks it would be best to have an event we could listen for when a frame content context goes away.  See https://bugzilla.mozilla.org/show_bug.cgi?id=582569.

Comment 42

[Justin Dolske [:Dolske]]

Comment on attachment 463701 [details] [diff] [review]
Patch with observer fixes

Is this patch ready for a final review? It's not flagged r?...

Comment 43

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

Comment on attachment 463701 [details] [diff] [review]
Patch with observer fixes

Let's get the review started. Work is underway to get the "unload" event for frame scripts, which would be used to remove the "earlyformsubmit" observer when the tab is closed.

We can interdiff that change, when it's ready.

Comment 44

[Justin Dolske [:Dolske]]

Comment on attachment 463701 [details] [diff] [review]
Patch with observer fixes

Oh, sigh, I forgot my own comment 34 when I wrote comment 42. :S

This patch just needs the addition to watch for the new bug 582569 event, but is otherwise fine. So, all that's needed is a review on the post-582569 changes and this should be good to go.

Comment 45

[Benjamin Stover (:stechz)]

Attachment: Unload listener

Dolske, if you r+ this could you also flag the other patch r+ if you think it is ready to land?

Comment 46

[Benjamin Stover (:stechz)]

Sorry for bug spam.

For some reason when using this with Fennec I cannot use const.  I think the global message managers for in process and out of process may require different things here?

Comment 47

[Benjamin Stover (:stechz)]

Sigh.  I am still seeing the pref observer being removed after some amount of time in Firefox.  I even changed this to a strong reference with no luck.  I think my best course of action now is to make a small test that can reproduce this problem and file a bug...

Comment 48

[Justin Dolske [:Dolske]]

Comment on attachment 465857 [details] [diff] [review]
Unload listener

>-const Cc = Components.classes;
>-const Ci = Components.interfaces;
>+var Cc = Components.classes;
>+var Ci = Components.interfaces;

Huh, can you file a followup for why this doesn't work in Fennec? Perhaps the frameScriptLoader is defaulting to JS 1.5, so newer JS features (like const, array extras, etc) are unavailable? That would be really good to fix -- not only to use them, but also to make sure Fennec doesn't keep breaking at random times when shared code changes and works fine on Firefox.

>+        addEventListener("unload", this, false);

The event name is a little unfortunate, since documents already fire an |unload| event and that's a warning flag (because it breaks fastback caching). But looks like this event (added in bug 582569) is completely unrelated. Does mean it's a little awkward should a script want both types of unload events via a single handleEvent + switch, but oh well.

Comment 49

[Justin Dolske [:Dolske]]

(In reply to comment #47)
> Sigh.  I am still seeing the pref observer being removed after some amount of
> time in Firefox.

Sadface. That'll need to be fixed before landing this, if for no other reason than pwmgr tests will fail randomly.

Comment 50

[Justin Dolske [:Dolske]]

(In reply to comment #49)
> pwmgr tests

Err, satchel tests. >_<

Comment 51

[Benjamin Stover (:stechz)]

I have just learned something terrible about preference observers.  See https://bugzilla.mozilla.org/show_bug.cgi?id=533290#c22.

Comment 52

[Benjamin Stover (:stechz)]

Attachment: Stop using pref branches

This patch sits on top of the other two patches.  Instead of grabbing the pref branch, this code always uses the root.  I also used strong refs because bug 533290 is still not resolved, just to be safe.

Comment 53

[Benjamin Stover (:stechz)]

Assuming this patch does well on try and you like this patch, we can land this baby!

Comment 54

[Benjamin Stover (:stechz)]

Attachment: Initialize services in Fennec

Comment 55

[Benjamin Stover (:stechz)]

Landed: http://hg.mozilla.org/projects/electrolysis/rev/116f2046b9ef
And then merged to m-c: http://hg.mozilla.org/mozilla-central/rev/116f2046b9ef

Comment 56

[Mike Shaver (:shaver emeritus)]

Implicated in a major sunspider regression (bug 588057), which is...odd, but not impossible.  Did this move Tss on the e10s Talos boxes?

Comment 57

[Robert Sayre]

(In reply to comment #56)
> Implicated in a major sunspider regression (bug 588057), which is...odd

And now Bug 588128.

Comment 58

[Robert Sayre]

Is this patch on the Fxb4 release branch?

Comment 59

[Mike Beltzner [:beltzner, not reading bugmail]]

Yes; I'd like it to be backed out of the Fxb4 release branch if possible.

Comment 60

[Mike Beltzner [:beltzner, not reading bugmail]]

Specifically, if I can get someone to back this out of GECKO20b4_20100817_RELBRANCH and confirm here, that'd be lovely.

Comment 61

[Robert Sayre]

(In reply to comment #56)
> Implicated in a major sunspider regression (bug 588057), which is...odd, but
> not impossible.  Did this move Tss on the e10s Talos boxes?

here's a V8 regression:

http://bit.ly/cbzHcY

The SunSpider data seems a little sparse. There may be a regression there too, though.

Comment 62

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #60)
> Specifically, if I can get someone to back this out of
> GECKO20b4_20100817_RELBRANCH and confirm here, that'd be lovely.

Turns out this isn't on the relbranch, so no need to worry about it.