1545513 - alert() in background script seems to block the extension

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[wolf+mozilla]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0

Steps to reproduce:

I create web extensions with background script

console.log("foo");
alert("test");

and load it as a temporary extension into clean profile via the debug extensions page. I do see foo in the console, however alert is not shown (based on https://bugzilla.mozilla.org/show_bug.cgi?id=1203394 I've believed the test string would show up).

But what is worse, the extensions seems to be now stuck. When I modify the background script to

console.log("foo");
console.log("bar");
alert("test");

and press reload, still only the foo is visible, so the extension is not reloaded. And when I try to turn off firefox (via exit in the menu), windows closes and developer console stays open. After I close that as well, I still see firefox as running in task manager and need to ctrl+c it from terminal to actually terminate.

Actual results:

Extension (and firefox) is kinda stuck after alert() in background script.

Expected results:

Extension (and firefox) is not kinda stuck after alert() in background script.

Comment 1

[wolf+mozilla]

This in terminal is maybe relevant (don't know):

[Parent 60, Main Thread] ###!!! ABORT: file resource://gre/modules/addons/XPIProvider.jsm, line 2207
[Parent 60, Main Thread] ###!!! ABORT: file resource://gre/modules/addons/XPIProvider.jsm, line 2207
Exiting due to channel error.
[Child 288, Chrome_ChildThread] WARNING: pipe error (3): Connection reset by peer: file /build/copy/src/firefox-65.0.2/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 349
Exiting due to channel error.
Exiting due to channel error.
Exiting due to channel error.

Comment 2

[Alin Ilea, Desktop QA]

Could you please attach the test extension?

Comment 3

[wolf+mozilla]

Basically all the code is in the original post already, but sure:

$ tree aa
aa
├── background.js
└── manifest.json

0 directories, 2 files

$ cat aa/background.js
console.log("foo");
alert("foo");
console.log("bar");

$ cat aa/manifest.json
{
  "description": "Foo",
  "manifest_version": 2,
  "name": "foo",
  "background": {
    "scripts": ["background.js"]
  },
  "version": "1.0"
}

Comment 4

[David Durst [:ddurst]]

You shouldn't use an alert() in a background script, so I can't say that this bug is valid.

However, knowing that we have tests for this, I'm leaving it open to see if there's anything on our side to look into.

(And that's the task, hence the categorization change.)

Comment 5

[wolf+mozilla]

You shouldn't use an alert() in a background script

Sure, I know that now :)

I can't say that this bug is valid.

So basically freezing whole extension without a way to reload it short of killing firefox and restarting is not "a bug"? If the alert() in background script caused segfault and firefox crashed, it would still not be a bug because I'm not supposed to use alert() in background script?

Also there is https://bugzilla.mozilla.org/show_bug.cgi?id=1203394 with resolution of VERIFIED FIXED, which based on https://bugzilla.mozilla.org/show_bug.cgi?id=1203394#c49 was supposed to show:

There is no alert shown, and the console shows "alert() is not supported in background windows; please use console.log instead."

And that did not happen, so I still thing this is a bug..

Comment 6

[Rob Wu [:robwu]]

I can reproduce this issue, including the hang: bp-041aa30c-104f-4a02-8594-35f490190501
Seems to hang at XPIProvider.cleanupTemporaryAddonsthe, just like https://bugzilla.mozilla.org/show_bug.cgi?id=1543354#c3 ; this is specific to temporary extensions, and happens when any error is thrown (independent of alert that is reported here). Let's focus this bug on the alert issue.

The reason that alert throws* is because the logic that is supposed to turn alert into the equivalent of console.log does not work: We overwrite alert too late. It is lazily set in the childManager getter, which is triggered when an extension API is accessed, and if that does not happen, at DOMContentLoaded.

We do have a test, but it calls an extension API before calling alert, which shadows the bug.

To fix this bug, we should move this logic to the constructor of ExtensionPageContextChild, and remove the browser.test.log call at the start of the test (maybe turn it into a console.log or dump call).

* It is easier to see the cause of the error when the extensions.webextensions.remote preference is set to false: Then the global JS console displays the following error:

NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDOMWindowUtils.isParentWindowMainWidgetVisible] Prompter.jsm:344

Comment 7

[Rob Wu [:robwu]]

This is a good bug for beginners to start with Firefox development, as the patch is just moving around a few lines of code and making a minor modification to a test (as described in comment 6).

To get started, see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp
If anyone has questions about contributing, just needinfo me.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The component has been changed since the priority was decided, so we're resetting it.
For more information, please visit auto_nag documentation.

Comment 9

[Myeongjun Go]

Hello Rob :)
This is exciting bug. When I work test case to solve another bug, I met this bug.
Can I work on it?

Comment 10

[Rob Wu [:robwu]]

Sure, take the bug.

Comment 11

[Myeongjun Go]

Attachment: Bug 1545513 - alert() in background script seems to block the extension. r=robwu

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb178f3b180d
alert() in background script seems to block the extension. r=robwu,mixedpuppy

Comment 13

[Razvan Maries]

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

Comment 14

[Madalin Cotetiu]

Verified in Win7x64 and Ubuntu 14.0.4 FF70.0a1(20190709153742) and 69.0b3
I have used an extension that using the following code in the background script:

console.log("before the alert 1");
console.log("before the alert 2");

alert("test");

console.log("after the alert 1");
console.log("after the alert 2");

All the console logs are correctly displayed and no hang or crash can be noticed due to the alert() being present in the script. Also a clear message stating that alert can't be used in background scripts is displayed "alert() is not supported in background windows; please use console.log instead." Reloading the extension does not cause any other hang or crash.

Marking bug as verified fixed.