1756495 - Dynamic content script registration may be incomplete when a new process spawns during the registration

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[Rob Wu [:robwu]]

A dynamically registered content script may not actually be registered correctly when a new process spawns during the registration process. Consequently, such content scripts may not actually execute in web pages that are hosted in that content process.

The current implementation of dynamic content script registrations has the following logic to register scripts:

1. Broadcast Extension:RegisterContentScript with the script options.
2. Update the shared memory with dynamic content script registrations.

Examples where content scripts are being registered:

• https://searchfox.org/mozilla-central/rev/211649f071259c4c733b4cafa94c44481c5caacc/toolkit/components/extensions/parent/ext-contentScripts.js#198-205
• https://searchfox.org/mozilla-central/rev/211649f071259c4c733b4cafa94c44481c5caacc/toolkit/components/extensions/parent/ext-userScripts.js#130-137
• https://phabricator.services.mozilla.com/D134154?id=541346#C4776460NL193

The registrations are used as follows:

• In existing processes, the registration is received via the Extension:RegisterContentScript messages and processed at https://searchfox.org/mozilla-central/rev/211649f071259c4c733b4cafa94c44481c5caacc/toolkit/components/extensions/ExtensionProcessScript.jsm#249-280
• In new processes, the shared memory is read at https://searchfox.org/mozilla-central/rev/211649f071259c4c733b4cafa94c44481c5caacc/toolkit/components/extensions/ExtensionProcessScript.jsm#178.

An issue occurs when a new process spawns between the (1) Extension:RegisterContentScript broadcast and the (2) shared memory update. In that case, the content script is not correctly registered and will therefore not run.
To fix this issue, the two should be reversed, and the implementation has to be revised to account for the swapped logic.

STR (simulation):

1. Launch Firefox with any extension with host permissions for example.com (e.g. via web-ext run).
2. Allow the extension access to private browsing mode via about:addons.
3. Visit about:debugging, find the extension and inspect its background page (or click on the manifest.json link to open a new tab and open the devtools for it).
4. Open the Browser Content Toolbox, search for ExtensionProcessScript.jsm and put a breakpoint at the Extension:RegisterContentScript handler to simulate a slow-to-respond process, at https://searchfox.org/mozilla-central/rev/211649f071259c4c733b4cafa94c44481c5caacc/toolkit/components/extensions/ExtensionProcessScript.jsm#253
5. Switch to the devtools from step 3, and run the following snippet:

s = await browser.contentScripts.register({
  css: [{code:"*{background:cyan}"}],
  matches: ["*://example.com/"],
})

6. The breakpoint should be triggered in the devtools from step 4.
7. Open a private browsing window, visit about:processes and look at the pre-allocated processes.
8. Repeat this step until every preallocated process that was initially shown at step 7 has disappeared (e.g. a few times): Visit example.com in a private window and close the window.

• This step is needed, because we want to test the behavior with a new process spawn. There may be pre-allocated processes that aren't rendering content, but are already processing messages.

9. Visit example.com in a private browsing window once more, and look at the background color.
10. Return to the devtools from step 4/6, and resume execution.
11. Refresh the tab from step 9, and look at the background color.

Expected:

• At step 8, 9 and 11, the background color should always be cyan.

Actual:

• At step 8, the first few attempts may indeed have a cyan background.
• At step 9, the background doesn't change, which means that the content script isn't registered in the process.
• At step 11, the background doesn't change, which means that the issue is persistent.

Comment 1

[William Durand [:willdurand]]

Attachment: Bug 1756495 - Ensure script registration is complete when a new process is spawned during the registration. r?robwu!

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6a3e5603500
Ensure script registration is complete when a new process is spawned during the registration. r=robwu

Comment 3

[Norisz Fay [:noriszfay]]

Backed out for causing xpcshell failures on test_ext_contentscript_dynamic_registration.js

Backout link

Push with failures

Failure log

Failure line(s): TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_dynamic_registration.js | test_contentScripts_register - [test_contentScripts_register : 96] expected PID 1 to not be in [0, 1]) - false == true

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ae4e33f83bc
Ensure script registration is complete when a new process is spawned during the registration. r=robwu

Comment 5

[Sandor Molnar[:smolnar]]

Backed out changeset 9ae4e33f83bc (bug 1756495) for causing xpc failures in xpcshell/test_ext_contentscript_dynamic_registration.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/2b935e00ff63f8a937e135ae77f04679ae97e08c

Push with failures

Failure log

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1d3464943fa
Ensure script registration is complete when a new process is spawned during the registration. r=robwu

Comment 7

[Andreea Pavel [:apavel]]

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