1926106 - moz-extension subframes created from generated_background_page.html fails to load when the addon is loaded temporarily as unpacked

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

I've noticed this issue last week while I was testing out this scenario (loading a moz-extension url into a background page subframe) last week while reviewing Bug 1754452, the behavior felt particularly surprising and so I wanted to dig more into it.

When an extension defines its background page using the background.scripts manifest property (instead of the background.page alternative way of defining the backgrond page) AND the extension is loaded temporarily as unpacked (instead of being loaded as packed inside an unsigned zip file or a AMO signed xpi file) then loading another moz-extension URL into a background page subframe fails and the following assertion failure is logged while hitting that in a debug build:

[Parent 1063125, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x80004002 (NS_NOINTERFACE): file /builds/worker/checkouts/gecko/netwerk/ipc/DocumentLoadListener.cpp:546

The moz-extension subframe instead loads just fine if either:

• the background page is defined using background.page (in that case it works and it doesn't matter if the addon is packed or unpacked)
• or the extension page is loaded as an unsigned zip file or a signed xpi package (in that case it works and it doesn't matter if the background page is defined through the background.page or background.scripts manifest property).

By comparing what happens internally for a Firefox instance where the subframe load failed and comparing it with one where it didn't fail, it looks that:

• the assertion error logged from DocumentLoadListener.cpp:546 is originated from mozilla::net::CheckRecursiveLoad in particular the nsresult rv = aLoadState->URI()->EqualsExceptRef(parentURI, &equal) call here is the one that will be raising the NS_NOINTERFACE error
• internally the call linked above is then going to be calling nsStandardURL::EqualsInternal and then returning the NS_NOINTERFACE nsresult from this line here, because a few lines before nsresult rv2 = other->EnsureFile(); did set rv2 to NS_NOINTERFACE
• the line rv2 = other->EnsureFile(); mentioned right above is going to be a call to SubstitutingURL::EnsureFile because moz-extension uris are SubstitutingURL instances, and that call ends up returning NS_NOINTERFACE from this line here because when the parent frame is the background page created internally when the extension is using background.scripts the background page will be the _generated_background_page.html page synthesized internally which doesn't actually exist on disk and so the scheme computed is "data" and then the method returns NS_NOINTERFACE (on the contrary when the extension is using background.page then the related extension page will be packaged with the extension and SubstitutingURL::EnsureFile will then return successfully and that's why the issue isn't hit in that case)
• nsStandardURL::EnsureFile virtual function has also an inline comment that briefly mention that under this condition EnsureFile is expected to return NS_NOINTERFACE (see inline comment here)

As a side note, here in nsStandardURL::EqualsInternal it looks like we do have a special case for resource URIs to avoid hitting similar issue for resource URIs that are not backed by actual files on disk, and so that may be also a place where we may consider to special case the _generated_background_page.html moz-extension urls for similar reasons.

I'll attach a reduced test case to hit the issue described in this bug from a small automated test.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1926106 - Add test case loading a moz-extension background page subframe in packed and unpacked extension. r?willdurand!

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1926106 - Special case _generated_background_page.html moz-extension url in nsStandardURL::EqualsInternal. r?valentin!

This patch includes a special handling for the moz-extension _generated_background_page.html urls inside
nsStandardURL::EqualsInternal because it doesn't resolve to a file (instead it resolves to a data: URI,
see ExtensionProtocolHandler::ResolveSpecialCases), similarly to resource:// urls which are special case
a few lines before in the same nsStandardURL::EqualsInternal.

The test case introduced by the parent patch passes successfully on the unpacked extension loaded temporarily
with these changes applied.

Depends on D226376

Depends on D226376

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46a246dc8c98
Add test case loading a moz-extension background page subframe in packed and unpacked extension. r=willdurand,valentin
https://hg.mozilla.org/integration/autoland/rev/b81ccaf28482
Special case _generated_background_page.html moz-extension url in nsStandardURL::EqualsInternal. r=valentin,necko-reviewers

Comment 4

[tszentpeteri]

Backed out for causing xpcshell failures on test_URIs.js

• Backout link
• Push with failures
• Failure Log:

[task 2024-10-24T16:40:22.302Z] 16:40:22     INFO -  TEST-PASS | toolkit/components/url-classifier/tests/unit/test_listmanager.js | took 1237ms
[task 2024-10-24T16:40:22.312Z] 16:40:22     INFO -  Retrying tests that failed when run in parallel.
[task 2024-10-24T16:40:22.316Z] 16:40:22     INFO -  TEST-START | netwerk/test/unit/test_URIs.js
[task 2024-10-24T16:40:23.178Z] 16:40:23  WARNING -  TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_URIs.js | xpcshell return code: 0
[task 2024-10-24T16:40:23.178Z] 16:40:23     INFO -  TEST-INFO took 862ms
[task 2024-10-24T16:40:23.179Z] 16:40:23     INFO -  >>>>>>>
[task 2024-10-24T16:40:23.184Z] 16:40:23     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2024-10-24T16:40:23.184Z] 16:40:23     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2024-10-24T16:40:23.185Z] 16:40:23     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2024-10-24T16:40:23.185Z] 16:40:23     INFO -  running event loop
[task 2024-10-24T16:40:23.187Z] 16:40:23     INFO -  netwerk/test/unit/test_URIs.js | Starting check_nested_mutations
[task 2024-10-24T16:40:23.187Z] 16:40:23     INFO -  (xpcshell/head.js) | test check_nested_mutations pending (2)
[task 2024-10-24T16:40:23.187Z] 16:40:23     INFO -  PID 17927 | TEST-INFO | /builds/worker/workspace/build/tests/xpcshell/tests/netwerk/test/unit/test_URIs.js | [do_check_uri_eq : 356] (uri equals check: 'about:blank' == 'about:blank')
[task 2024-10-24T16:40:23.187Z] 16:40:23     INFO -  TEST-PASS | netwerk/test/unit/test_URIs.js | check_nested_mutations - [check_nested_mutations : 357] true == true

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Valentin,
Quite unfortunately test_URIs.js didn't got selected and executed in my mach try auto and so I didn't spot this test was going to hit a failure was going to hit a failure due to the change in the expected vd actual behavior under that case.

The test task failing is the one named test_bug1875119 and in particular this assertion expecting the call to uri1.equals(uri2) to be throwing: https://searchfox.org/mozilla-central/rev/dca2603d55b5b39d3b8ab8e93c08b42563f5aad8/netwerk/test/unit/test_URIs.js#1032-1036

With the changes in the form we settled on at the moment the test doesn't hit the crash but it doesn't get an exception neither, instead uri1.equals(uri2) returns false as expected because the two underlying paths actually differ.

Before planning any actual change, I'd like to gather your perspective about what would be the right way to handle this test failure? e.g.:

• should we change the assertion to expect that to return false now (and if that's he case, do we need to add a new test case to keep some coverage that would not be explictly covered anymore? should we change the name of the test task into test_bug1875119_bug1926106?)
• or should we change the implementation to still raise the error under that case? (while still keep the behavior we want to have to fix the issue with the moz-extension _generated_background_page.html urls)

Let me know what you think.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #5)

Before planning any actual change, I'd like to gather your perspective about what would be the right way to handle this test failure? e.g.:

• should we change the assertion to expect that to return false now (and if that's he case, do we need to add a new test case to keep some coverage that would not be explictly covered anymore? should we change the name of the test task into test_bug1875119_bug1926106?)

Feel free to change the test to expect false. I'm OK with changing the name to reference both bug numbers (or even better you could add a comment explaining the history).
That corner case should be eliminated when we fix bug 1876483.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Valentin Gosu [:valentin] (he/him) {{ PTO until Nov 5 }} from comment #6)

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #5)

Before planning any actual change, I'd like to gather your perspective about what would be the right way to handle this test failure? e.g.:

• should we change the assertion to expect that to return false now (and if that's he case, do we need to add a new test case to keep some coverage that would not be explictly covered anymore? should we change the name of the test task into test_bug1875119_bug1926106?)

Feel free to change the test to expect false. I'm OK with changing the name to reference both bug numbers (or even better you could add a comment explaining the history).
That corner case should be eliminated when we fix bug 1876483.

ah, I see.

In the meantime I'm going to tweak the test to expect false. I agree that adding a comment may be useful (and be more immediately clear then just including this bug number in the test task function name), and so I'll add one.

(I may leave the test task name as is instead, at least if it feels that the inline comment would be enough to make it clear why the test doesn't expect an exception).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23e004500860
Add test case loading a moz-extension background page subframe in packed and unpacked extension. r=willdurand,valentin
https://hg.mozilla.org/integration/autoland/rev/1ff17c558ecf
Special case _generated_background_page.html moz-extension url in nsStandardURL::EqualsInternal. r=valentin,necko-reviewers

Comment 9

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/23e004500860
https://hg.mozilla.org/mozilla-central/rev/1ff17c558ecf