1128760 - Content javascript exceptions cause Marionette JavascriptException during chrome context execute_script

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Eric Rahm [:erahm]]

When executing a script via |self.marionette.execute_async_script| [1] in the chrome context I'm getting a JavascriptException for an unrelated javascript (in this case it's reporting an exception in one of the loaded pages).

Example code:

>        gc_script =  """
>            const Cu = Components.utils;
>            const Cc = Components.classes;
>            const Ci = Components.interfaces;
>            
>            Cu.import("resource://gre/modules/Services.jsm");
>            Services.obs.notifyObservers(null, "child-mmu-request", null);
>            let memMgrSvc = Cc["@mozilla.org/memory-reporter-manager;1"].getService(Ci.nsIMemoryReporterManager);
>            memMgrSvc.minimizeMemoryUsage(() => marionetteScriptFinished("gc done!"));
>            """
>        result = None
>        try:
>          result = self.marionette.execute_async_script(gc_script, script_timeout=180000);
>        except JavascriptException, e:
>          self.logger.error("GC JavaScript error: %s" % e)

And the exception that gets logged:
>18:29.92 LOG: MainThread ERROR GC JavaScript error: JavascriptException: TypeError: Argument 1 of Window.getComputedStyle is not an object. at: http://localhost:8095/tp5/aljazeera.net/widgets.twimg.com/j/2/widget.js line: 53


[1] https://github.com/EricRahm/areweslimyet/blob/marionette/benchtester/test_memory_usage.py#L191

Comment 1

[Eric Rahm [:erahm]]

Attachment: test_exceptions.py

Simple marionette test case that reproduces the issue.

Comment 2

[Jonathan Griffin (:jgriffin)]

This behavior is by design.  Basically, when we invoke an async script, we have to register a window.onerror handler, and we can't tell whether any errors that occur originated with the script being executed or something else (like the current page), since they occur in the same context.

We've talked in the past about the possibility of adding an option to _not_ register this handler, but we haven't (AFAICT) implemented it.

Comment 3

[Eric Rahm [:erahm]]

(In reply to Jonathan Griffin (:jgriffin) from comment #2)
> This behavior is by design.  Basically, when we invoke an async script, we
> have to register a window.onerror handler, and we can't tell whether any
> errors that occur originated with the script being executed or something
> else (like the current page), since they occur in the same context.
> 
> We've talked in the past about the possibility of adding an option to _not_
> register this handler, but we haven't (AFAICT) implemented it.

It's surprising that errors from the content context are getting mixed in w/ errors in the chrome context.

Unfortunately this blocks AWSY's switch to marionette, so some sort of option to at least disable this behavior would be useful.

Comment 4

[Jonathan Griffin (:jgriffin)]

Hmm good point, I missed the chrome/content distinction in the original description.  For chrome context, it seems like we should register the onerror handler for the browser window, rather than the contentWindow, as we do now.

Comment 5

[Eric Rahm [:erahm]]

(In reply to Jonathan Griffin (:jgriffin) from comment #4)
> Hmm good point, I missed the chrome/content distinction in the original
> description.  For chrome context, it seems like we should register the
> onerror handler for the browser window, rather than the contentWindow, as we
> do now.

I'm happy to test a fix for this. It looks like you're referring to code in marionette-server.js [1], in this context is the "browser window" curBrowser?

[1] https://hg.mozilla.org/mozilla-central/annotate/7c87286ce969/testing/marionette/marionette-server.js#l1094

Comment 6

[Jonathan Griffin (:jgriffin)]

Yep, that's it!

Comment 7

[Eric Rahm [:erahm]]

Attachment: WIP: Content javascript exceptions cause Marionette JavascriptException during chrome context execute_script

WIP - attempted to use curBrowser, but this doesn't seem to be what we want
(the errors from the content are still propagated).

Comment 8

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8558811 [details] [diff] [review]
WIP: Content javascript exceptions cause Marionette JavascriptException during chrome context execute_script

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

lgtm!  Does it pass a try run?

Comment 9

[Eric Rahm [:erahm]]

(In reply to Jonathan Griffin (:jgriffin) from comment #8)
> Comment on attachment 8558811 [details] [diff] [review]
> WIP: Content javascript exceptions cause Marionette JavascriptException
> during chrome context execute_script
> 
> Review of attachment 8558811 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm!  Does it pass a try run?

I should have been clearer, this doesn't work. We still get exceptions from the content script. I was hoping you might have another suggestion.

Comment 10

[Jonathan Griffin (:jgriffin)]

I played around with this a bit.  You're right, the content errors get bubbled up to the browser's onerror handler.  Also interesting is that JS errors that occur in chrome context are not caught by this handler.

I think we should probably just disable the onerror stuff for chrome, since the current implementation doesn't make sense.  We can look at adding it back later if we can figure out how to trap chrome-space async errors.

Comment 11

[Eric Rahm [:erahm]]

Attachment: Part 1: Disable onerror hooking in chrome context

Comment 12

[Eric Rahm [:erahm]]

Attachment: Part 2: Exposed debug_script option

Comment 13

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09d77f189d54

Comment 14

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8560915 [details] [diff] [review]
Part 1: Disable onerror hooking in chrome context

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

lgtm.  David, are you ok with this approach?

Comment 15

[David Burns :automatedtester]

Comment on attachment 8560915 [details] [diff] [review]
Part 1: Disable onerror hooking in chrome context

yea, that looks good

Comment 16

[Eric Rahm [:erahm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a19cb997c026
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/075d31f8da0e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a19cb997c026
https://hg.mozilla.org/mozilla-central/rev/075d31f8da0e