1814293 - test_ext_contentscript_errors.js fails with parent controlled navigation

Status: RESOLVED FIXED

(WebExtensions :: General, task, P3)

Description

[Andrew McCreight [:mccr8]]

When the pref browser.tabs.documentchannel.parent-controlled is set to true, the test test_ext_contentscript_errors.js fails.

The main failure is that the call to nsIConsoleService.unregisterListener is failing with the helpful error NS_ERROR_FAILURE. If you comment out that line, it instead fails a little later with "TypeError: can't access property "length", errors is undefined", apparently because the value of this.collectedErrors is undefined when it is returned.

This test was added in bug 1410932.

Comment 1

[Andrew McCreight [:mccr8]]

The ContentTask.spawn for the unregisterListener is ending up in a different process than the one that does the registerListener, so it is no wonder things are going awry. I'll have to look over some older issues that have been fixed for parent controlled navigation as that sounds kind of familiar.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1814293 - Eagerly report the errors in test_ext_contentscript_errors.js.

The main goal of the changes is to make this test pass when parent-controlled
navigation is enabled.

• Eagerly send collectedErrors back to the parent process when we get the
  expected number of errors. I think this avoids issues that might arise if
  parent-controlled navigation cause a process switch to happen earlier. It has
  the drawback that if we don't get enough errors we'll hit a time out. We wait
  for this reply using a Promise.

• Store collectedErrors on the listener object instead of the message manager.
  I don't know if this matters, but hopefully it has a less weird lifetime than
  the message manager object being used before.

• Use an early return in the listener as that ends up with a bit less
  indentation.

• I added some basic checking for the error messages: two messages in a row
  shouldn't have the same error message. An earlier version of the patch was
  reporting the same error 7 times, and this checking would catch that.

• Use registerCleanupFunction to unload the extension to reduce the amount of
  noise when there's a failure.

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #1)

The ContentTask.spawn for the unregisterListener is ending up in a different process than the one that does the registerListener, so it is no wonder things are going awry. I'll have to look over some older issues that have been fixed for parent controlled navigation as that sounds kind of familiar.

I looked at this again, and I think I was confused about this. The unregister listener is running in the same process as we did the registerListener, which is also in the same process as TEST_URL_1 and TEST_URL_2. I think the immediate issue of the failure is that the state on the this object that the ContentTask function gets was cleared somehow, so this.collectedErrors and this.consoleErrorListener are undefined. I'm not sure why that is.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6220cf83d28f
Eagerly report the errors in test_ext_contentscript_errors.js. r=extension-reviewers,robwu

Comment 5

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/6220cf83d28f