Henrik Skupin [:whimboo][⌚️UTC+2] Reporter
	Description • 10 years ago

With e10s enabled our waitForPageLoad method is failing for any page. As result we always run into a timeout.

ERROR | Test Failure | {
  "exception": {
    "message": "controller.waitForPageLoad(URI=http://localhost:43336/cookies/cookie_single.html, readyState=complete)", 
    "lineNumber": 27, 
    "name": "AssertionError", 
    "fileName": "resource://mozmill/modules/errors.js"
  }
}

Interestingly we have seen similar failure messages for our mozmill-tests with e10s disabled but very rarely.

	Szabolcs Hubai (:xabolcs)
	Comment 1 • 10 years ago

After a quick research we need to do here (in 'resource://mozmill/modules/windows.js')
is the same what the "Run a script in all pages" example [1] does: load a frame script [2]
which catches the page load and notifies the chrome (i.e. mozmill) code.

Uncommenting the dump statements in windows.js, you can see: only the chrome page loads are intercepted (preferences.xul, browser.xul).

For more about loading frame scripts please read about the message manager MDN page [3].




[1]: https://developer.mozilla.org/en-US/Add-ons/Working_with_multiprocess_Firefox#Run_a_script_in_all_pages
[2]: https://github.com/mdn/e10s-example-addons/tree/master/run-script-in-all-pages
[3]: https://developer.mozilla.org/en-US/docs/The_message_manager

	Szabolcs Hubai (:xabolcs)
	Comment 2 • 10 years ago

(In reply to Szabolcs Hubai (:xabolcs) from comment #1)
> After a quick research we need to do here (in
> 'resource://mozmill/modules/windows.js')
> is the same what the "Run a script in all pages" example [1] does: load a
> frame script [2]
> which catches the page load and notifies the chrome (i.e. mozmill) code.
> 

My toughts about how to fix this bug are the following.

The key in this bug is that 'resource://mozmill/modules/windows.js' "needs to factor the code
that needs direct access to content into separate scripts, which are called 'frame scripts'" as the tutorial says.

So the first interesting topic is what to slice out from 'windows.js' into (for example) 'windows-frame.js'.
I think, the eventlistener-manager part (mostly attachEventListeners()'s content) should go to a frame script.

The second interesting topic is to establish an API between 'windows-frame.js' and 'windows.js'.
The frame script should have a way to notify the chrome script about the listened event:
"load", "pagehide", "pageshow", "beforeunload", "DOMContentLoaded".
Of course 'windows.js' should be updated to know about the message manager,
and should use |addMessageListener()| to get those messages from 'windows-frame.js'.

The third interesting topic is the backward compatibility.
The previously linked addon works with beta release of Firefox, currently 33.0.
It should be tested against older versions, especially ESR.
It worths to note that addon runs fine for chrome pages too.

The key topic in the communication API would be that how the frame script could pass 
an identification about the listened window that the chrome script understands:
|utils.getWindowId()| doesn't work in a frame script.
I think the message manager docs will help us here.

	Henrik Skupin [:whimboo][⌚️UTC+2] Reporter
	Comment 3 • 10 years ago

(In reply to Szabolcs Hubai (:xabolcs) from comment #1)
First, you did some great investigation here over the weekend! Thanks a lot for finding all those kinda helpful links. This will indeed help a lot.

> After a quick research we need to do here (in
> 'resource://mozmill/modules/windows.js')
> is the same what the "Run a script in all pages" example [1] does: load a
> frame script [2]
> which catches the page load and notifies the chrome (i.e. mozmill) code.

This sounds like the right approach and something what we missed when the waitForPageLoad code got extended a while ago. 

> Uncommenting the dump statements in windows.js, you can see: only the chrome
> page loads are intercepted (preferences.xul, browser.xul).

So means the same listeners for unload, load etc we also need for the content scope?

> So the first interesting topic is what to slice out from 'windows.js' into
> (for example) 'windows-frame.js'.
> I think, the eventlistener-manager part (mostly attachEventListeners()'s
> content) should go to a frame script.

The question here first is, how do about: pages behave? I assume all events in there are chrome, or am I wrong? If those are in chrome scope we may have to duplicate the file, or being able to load the same module into different scopes.

> The second interesting topic is to establish an API between
> 'windows-frame.js' and 'windows.js'.
> The frame script should have a way to notify the chrome script about the
> listened event:
> "load", "pagehide", "pageshow", "beforeunload", "DOMContentLoaded".
> Of course 'windows.js' should be updated to know about the message manager,
> and should use |addMessageListener()| to get those messages from
> 'windows-frame.js'.

Yeah. A long time ago we already started with that, and as result the message broker has been added. You might want to check some code of other existent modules, which most likely already exchanges code that way.

> The third interesting topic is the backward compatibility.
> The previously linked addon works with beta release of Firefox, currently
> 33.0.
> It should be tested against older versions, especially ESR.
> It worths to note that addon runs fine for chrome pages too.

The lowest number of Firefox / Gecko we currently support is Firefox 24ESR. But support will end soon. If we can get it to work without any workarounds, would be good. Otherwise I wouldn't care that much about it.

> The key topic in the communication API would be that how the frame script
> could pass 
> an identification about the listened window that the chrome script
> understands:
> |utils.getWindowId()| doesn't work in a frame script.
> I think the message manager docs will help us here.

Yes, the window id we have to retrieve from chrome. So the messagebroker should really help here I hope.

	Szabolcs Hubai (:xabolcs)
	Comment 4 • 10 years ago

Just uploading wbamberg's example addon injected with Mozmill's windows.js.

I uncommented almost all the dump statements. Should I uncomment the last two (map.update(), map.updatePageLoadStatus()) too?

	Szabolcs Hubai (:xabolcs)
	Comment 5 • 10 years ago

Current version of the addon.

The greatest news is that the chrome script is able to respond an outerWindowID to frame script. :)


Packed from the current head [1] of 'branch-bug-1060238-mm-poc' branch [2].

[1] https://github.com/xabolcs/e10s-example-addons/tree/185e88dade10112c791f8e611cdabc73e1b9f9bd/run-script-in-all-pages/ported-message-manager/src
[2] https://github.com/xabolcs/e10s-example-addons/tree/branch-bug-1060238-mm-poc/run-script-in-all-pages/ported-message-manager/src

	Henrik Skupin [:whimboo][⌚️UTC+2] Reporter
	Comment 6 • 10 years ago

Thanks Szabolcs! I think it is worth moving all this out to a new bug, which is about implementing support for the message manager. I created bug 1068678 for that purpose.

	Szabolcs Hubai (:xabolcs)
	Comment 7 • 10 years ago

A test to reproduce the error:
- it enables browser.tabs.remote and browser.tabs.remote.autostart prefs and restarts Firefox
- loads about: then waits 3 seconds
- loads nightly.mozilla.org then waits 3 seconds

	Szabolcs Hubai (:xabolcs)
	Comment 8 • 10 years ago

(In reply to Henrik Skupin (:whimboo) from comment #0)
> With e10s enabled our waitForPageLoad method is failing for any page. As
> result we always run into a timeout.
> 
> ERROR | Test Failure | {
>   "exception": {
>     "message":
> "controller.waitForPageLoad(URI=http://localhost:43336/cookies/cookie_single.
> html, readyState=complete)", 
>     "lineNumber": 27, 
>     "name": "AssertionError", 
>     "fileName": "resource://mozmill/modules/errors.js"
>   }
> }
> 
> Interestingly we have seen similar failure messages for our mozmill-tests
> with e10s disabled but very rarely.

I'm trying to reproduce the timeout error with the attachment in comment #7, but always got this for Nightly 35.0a1 (Mozilla/5.0 (X11; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140918030202 CSet: 426497473505):

ERROR | Test Failure | {
  "exception": {
    "message": "win.document is undefined", 
    "lineNumber": 987, 
    "name": "TypeError", 
    "fileName": "resource://mozmill/driver/controller.js"
  }
}


And a slightly other one with Aurora 34.0a2 (Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140918004100 CSet: 7ff763eb328b):

ERROR | Test Failure | {
  "exception": {
    "message": "win is null", 
    "lineNumber": 987, 
    "name": "TypeError", 
    "fileName": "resource://mozmill/driver/controller.js"
  }
}


But never a timeout error. Both for mozmill master and 2.0.8.

	Szabolcs Hubai (:xabolcs)
	Comment 9 • 10 years ago

This is just a proof of concept patch to windows.js, based on the previously attached addon.

	Szabolcs Hubai (:xabolcs)
	Comment 10 • 10 years ago

This part of the POC patch creates a basic Messenger API and updates controller.waitForPageLoad() to use it.

	Szabolcs Hubai (:xabolcs)
	Comment 11 • 10 years ago

These proof of concept patches are against current mozmill@master [1].
For example I kept the dump statements enabled in windows-frame-script.js
But they fix waitForPageLoad(). :)

mozmill -b /path/to/nighly -t testEnableE10s.js

RESULTS | Passed: 3
RESULTS | Failed: 0
RESULTS | Skipped: 0



[1]: https://github.com/mozilla/mozmill/commit/95f24c5afa.

	Henrik Skupin [:whimboo][⌚️UTC+2] Reporter
	Comment 12 • 10 years ago

So we had a meeting lately together with the A-Team about the e10 implementation work for Mozmill. As it turned out the work to do here is way too much, and we should better focus on getting Marionette ready for replacing Mozmill as what we wanted to do anyway. Therefore we have a new project up here: https://wiki.mozilla.org/Auto-tools/Projects/m21s.

I'm going to close this bug as wontfix.

What I don't wanna forget is to say thank you Szabolcs for creating the POC! It give an insight of what all has to be worked on.