Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 7 years ago

Right now we are forwarding each content related command to the listener.js frame script which runs in a content process. As result we add a huge overhead especially for debug builds where the sending of the data takes a huge amount of time. Also if the framescript gets reloaded, all of the non-navigation related commands are affected by hangs.

To reduce those risks I would suggest that we move as much code as possible from the frame script to the parent process (driver.js). A good example are the changes for `getCurrentUrl` (bug 1368101).

Andreas, do you have any other comments on that? If not I can have a look at the various commands, and I can create mentored bugs for each of those.

	Andreas Tolfsen ❲:ato❳
	Comment 1 • 7 years ago

In principle I think it is worth reducing the number of calls to the
content frame script for the reasons you say.  It is also problematic
that a frame script may be blown away at any point due to a remoteness
change, and it is also problematic to store any kind of state in
listener.js for these reasons.

Tangentially, we have a cache invalidation problem with setting timeout
durations through the setTimeouts command because they are propagated to
the frame script when it is loaded through copying over the capabilities
object.  A better approach here would be to have some kind of querying
service in driver.js so that all state could be kept in one, irrevocable
place.  I believe you have already filed a bug on this in the past.

It would be _great_ if we could do all navigation and monitoring of load
events in chrome context, but I believe you had issues with this in the
past?  In light of recent changes, is this something you want to look
at?  Could we initiate a navigation and listen for the correct events
in chrome context, it would eliminate a lot of complexity (for example
this.curBrowser.pendingCommands).

OTOH other commands that are good candidates for living in chrome
context are:

getTitle: Probably an easy conversion because the title should be
directly accessible on the <xul:browser> or <tab> elements.

setTestName: Can maybe be removed altogether?  Not really sure why
we’re sending it to the frame script.

addCookie, getCookies, deleteAllCookies, deleteCookie: Should review
whether we can set directly from chrome space.  We currently call the
frame script, which calls the cookie service in chrome to actually
perform the cookie store manipulation.

The legacy action commands will be removed soonish, so they can be
ignored safely.  I don’t think we can do anything about getPageSource,
but maybe worth looking into.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #1)
> Tangentially, we have a cache invalidation problem with setting timeout
> durations through the setTimeouts command because they are propagated to
> the frame script when it is loaded through copying over the capabilities
> object.  A better approach here would be to have some kind of querying
> service in driver.js so that all state could be kept in one, irrevocable
> place.  I believe you have already filed a bug on this in the past.

Yes, but all that is actually the other way around. This bug is specifically for any unnecessary call from the driver to the listener.

> It would be _great_ if we could do all navigation and monitoring of load
> events in chrome context, but I believe you had issues with this in the
> past?  In light of recent changes, is this something you want to look
> at?  Could we initiate a navigation and listen for the correct events
> in chrome context, it would eliminate a lot of complexity (for example
> this.curBrowser.pendingCommands).

Yes, there were issues with missing events, and I still think that this is the same with my new implementation of the loadListener. But, I haven't tested that yet, and it's not a priority at the moment.

> OTOH other commands that are good candidates for living in chrome
> context are:
> 
> getTitle: Probably an easy conversion because the title should be
> directly accessible on the <xul:browser> or <tab> elements.

Yes, this one will be next. 

> setTestName: Can maybe be removed altogether?  Not really sure why
> we’re sending it to the frame script.

That is getting removed with the patch on bug 1368674 soon. So nothing we have to worry about.

> addCookie, getCookies, deleteAllCookies, deleteCookie: Should review
> whether we can set directly from chrome space.  We currently call the
> frame script, which calls the cookie service in chrome to actually
> perform the cookie store manipulation.

Not that sure given that this needs the current document, which is not available for the parent process.

> The legacy action commands will be removed soonish, so they can be
> ignored safely.  I don’t think we can do anything about getPageSource,
> but maybe worth looking into.

Maybe there is a browser internal property which handles the messaging itself. I would tend to say that this is more stable than our current messaging. :) So yes, we can have a look.

	Andreas Tolfsen ❲:ato❳
	Comment 3 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #2)

> (In reply to Andreas Tolfsen ‹:ato› from comment #1)
> 
> > addCookie, getCookies, deleteAllCookies, deleteCookie: Should
> > review whether we can set directly from chrome space.  We currently
> > call the frame script, which calls the cookie service in chrome to
> > actually perform the cookie store manipulation.
> 
> Not that sure given that this needs the current document, which is not
> available for the parent process.

The deleteCookie, deleteAllCookies, and getCookies functions in
testing/marionette/listener.js looks to be calling chrome directly
without any involvement of the document.

addCookie involves the document, but as far as I can tell only for
document.location.host, which is also available in chrome.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 4 • 7 years ago

Looks like lot of cleanup happened here in the last days. So the only remaining command is `getTitle`. Also `getAppCacheStatus` seems to be unused for a while now and can safely be related.

	David Burns :automatedtester
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8882864 [details]
Bug 1368439 - Retrieve content browser title via parent process.

https://reviewboard.mozilla.org/r/153916/#review159080

	David Burns :automatedtester
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8882865 [details]
Bug 1368439 - Remove unused GetAppCacheStatus command.

https://reviewboard.mozilla.org/r/153918/#review159082

<3

	Pulsebot
	Comment 11 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5cc825c0c008
Remove unused GetAppCacheStatus command. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/83f8dd309cd4
Retrieve content browser title via parent process. r=automatedtester

	Carsten Book [:Tomcat]
	Comment 12 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5cc825c0c008
https://hg.mozilla.org/mozilla-central/rev/83f8dd309cd4