979051 - nsIFormFillController errors running mochitest-browser tests with e10s enabled.

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Bug 897061 was to fix formfill issues with e10s, but running tests with e10s causes lots of tests to fail like so:

0:05.50 TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/general/browser_URLBarSetURI.js | Loading tab
 0:05.54 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_URLBarSetURI.js | uncaught exception - NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIFormFillController.attachToBrowser] at chrome://global/content/bindings/browser.xml:512
...
0:08.38 TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug304198.js | Console message: [JavaScript Error: "NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIFormFillController.attachToBrowser]" {file: "chrome://global/content/bindings/browser.xml" line: 512}]
 0:08.38 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug304198.js | uncaught exception - NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIFormFillController.attachToBrowser] at chrome://global/content/bindings/browser.xml:512
...

etc.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Don't add pageshow and pagehide handlers if no docshell

There is a good chance these are only seen while running tests - browser.xml adds pageshow and pagehide handlers (which rely on having a docShell).  The test infra simulates these events to help tests that also hook them.  So my guess is that when running tests, these events fire when they don't otherwise.

This patch solves it, but I'm not sure if it is appropriate.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8384986 [details] [diff] [review]
Don't add pageshow and pagehide handlers if no docshell

Review of attachment 8384986 [details] [diff] [review]:
-----------------------------------------------------------------

Hm I'm unsure about this too.. I want to give it a try at running the test and see what's up. I don't know which browser wouldn't have a docshell.. I think they all do, and maybe what this patch does is just causing it to be initialized earlier..  Which could point that the problem is that the e10s test infra is sending the pageshow/pagehide too early? Just a different theory..

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Needinfo me to not forget about this

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Felipe Gomes from comment #2)
> Hm I'm unsure about this too.. I want to give it a try at running the test
> and see what's up. I don't know which browser wouldn't have a docshell.. I
> think they all do,

hrm - I thought we'd expect *none* of them to have a docShell for browsers with remote="true"?

> and maybe what this patch does is just causing it to be
> initialized earlier..  Which could point that the problem is that the e10s
> test infra is sending the pageshow/pagehide too early? Just a different
> theory..

I believe that what's happening is that even for remote frames, browser.xml adds pageShow and pageHide handlers - but it never actually sees these events fire for remote browsers, as the event fires in the child.

However, the e10s test hacks add an event handler for these events to the child process, and "forwards" them to the parent.  Suddenly these remote browsers *are* seeing the events and blow up.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Blocking bug 932142 as this prevents tests passing for me.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8384986 [details] [diff] [review]
Don't add pageshow and pagehide handlers if no docshell

Review of attachment 8384986 [details] [diff] [review]:
-----------------------------------------------------------------

Ok yeah, I was confused about how this was being used. I don't know if this is the ideal fix or just a workaround, but if this gets the tests going, let's do it for now.  But use this.isRemoteBrowser instead of this.docShell.

Bill, was it you who worked on the feeds code? Do you know if the feeds code from browser.xml onPageHide is handled somewhere else for e10s, or if this is something missing/left-over? (I can't easily check at the moment)

I'm trying to think if the code called by onPageShow/onPageHide should handle this better itself, or if maybe remote-browser.xml should override these functions to do nothing, or if just not registering these listeners is the best way to go.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Thanks!

(In reply to :Felipe Gomes from comment #6)

> Bill, was it you who worked on the feeds code? Do you know if the feeds code
> from browser.xml onPageHide is handled somewhere else for e10s, or if this
> is something missing/left-over? (I can't easily check at the moment)

IIUC, formfill for e10s is done via http://mxr.mozilla.org/mozilla-central/source/toolkit/content/browser-child.js#357.  In the ideal world, it seems we should be able to load this code via a frame-script in both the remote and local browser cases.  However, on IRC, Bill mentioned there was some non-trivial issue with this - "I think the main problem is that the form controller code works differently in e10s."

So it seems we should probably take this bug now, but file a followup to unify the form-fill code in the future.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to :Felipe Gomes from comment #6)
> Bill, was it you who worked on the feeds code? Do you know if the feeds code
> from browser.xml onPageHide is handled somewhere else for e10s, or if this
> is something missing/left-over? (I can't easily check at the moment)

I think feeds are still broken in e10s. I just filed bug 989176 for this.

> I'm trying to think if the code called by onPageShow/onPageHide should
> handle this better itself, or if maybe remote-browser.xml should override
> these functions to do nothing, or if just not registering these listeners is
> the best way to go.

I think the best way to handle this is to move more of this code to content scripts. The middle click scrolling thing was the first case where we did that with browser.xml code (and that page was able to remove some pageshow/pagehide code). As we do more of that, we should be able to completely eliminate these handlers from browser.xml. All the code should migrate to toolkit/content/browser-content.js, which is loaded in e10s and non-e10s.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9277b148510

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d9277b148510