1313440 - 2,900 instances of "NS_ENSURE_TRUE(IsSandbox(sb)) failed" emitted from js/xpconnect/src/XPCComponents.cpp during linux64 debug testing

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Eric Rahm [:erahm]]

> 2926 WARNING: NS_ENSURE_TRUE(IsSandbox(sb)) failed: file js/xpconnect/src/XPCComponents.cpp, line 3024

This warning [1] shows up in the following test suites:

>   2757 - desktop-test-linux64/debug-mochitest-jetpack JP
>    114 - desktop-test-linux64/debug-mochitest-devtools-chrome-9 dt9
>     55 - desktop-test-linux64/debug-mochitest-chrome-3 c3

It shows up in 41 tests. A few of the most prevalent:

>    198 -        page-mod-debugger-post/main.testDebugger
>    110 -        devtools/client/debugger/test/mochitest/browser_dbg_listaddons.js
>    106 -        places/main.testVisitCount
>    100 -        content-permissions/main.testCrossDomainXHR
>    100 -        content-script-messages-latency/main.testContentScriptLatencyRegression
>     99 -        test-require/main.testSDKRequire
>     98 -        simple-prefs/main.testOptionsType
>     98 -        simple-prefs-l10n/main.testAOMLocalization
>     94 -        chrome/main.testChromeSkin
>     93 -        tab-close-on-startup/main.testNoTabCloseOnStartup

[1] https://hg.mozilla.org/mozilla-central/annotate/f9f3cc95d728/js/xpconnect/src/XPCComponents.cpp#l3024

Comment 1

[Bobby Holley (:bholley)]

That seems to suggest that the jetpack code passes something that's not a sandbox to nukeSandbox, which is probably a bug.

Comment 2

[Andrew McCreight [:mccr8]]

Regression from bug 1309351?

Comment 3

[Kris Maglione [:kmag]]

This is mainly just an issue with the legacy bootstrap code that's still used by the Jetpack tests. I guess we should just check whether the module has its own sandbox from JS rather than catching the exception that nukeSandbox throws.

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1313440: Fix NS_ENSURE_TRUE(isSandbox) warnings in test code.

Review commit: https://reviewboard.mozilla.org/r/89036/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89036/

Comment 5

[Kris Maglione [:kmag]]

Attachment: Fix NS_ENSURE_TRUE(isSandbox) warnings in JPM bootstrap.

Review commit: https://reviewboard.mozilla.org/r/89038/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89038/

Comment 6

[Kris Maglione [:kmag]]

Let me know if you want me to redirect this review, but the changes are pretty simple, and the code doesn't really have an owner at this point.

Comment 7

[Eric Rahm [:erahm]]

Comment on attachment 8805257 [details]
Bug 1313440: Fix NS_ENSURE_TRUE(isSandbox) warnings in test code.

https://reviewboard.mozilla.org/r/89036/#review88182

::: addon-sdk/source/app-extension/bootstrap.js:301
(Diff revision 1)
>    scriptLoader.loadSubScript(uri, sandbox, 'UTF-8');
>    return sandbox;
>  }
>  
>  function unloadSandbox(sandbox) {
> -  if ("nukeSandbox" in Cu) {
> +  if (Cu.getClassName(sandbox, true) == "Sandbox")

I guess nukeSandbox is always a thing at this point? Can it throw or is that not a concern?

Comment 8

[Eric Rahm [:erahm]]

Comment on attachment 8805257 [details]
Bug 1313440: Fix NS_ENSURE_TRUE(isSandbox) warnings in test code.

https://reviewboard.mozilla.org/r/89036/#review88184

Comment 9

[Eric Rahm [:erahm]]

Comment on attachment 8805258 [details]
Fix NS_ENSURE_TRUE(isSandbox) warnings in JPM bootstrap.

https://reviewboard.mozilla.org/r/89038/#review88186

Ah right it shouldn't throw because we've verified it's a sandbox. If you can to a try run with the JP tests I can verify the warnings are gone.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8805257 [details]
Bug 1313440: Fix NS_ENSURE_TRUE(isSandbox) warnings in test code.

https://reviewboard.mozilla.org/r/89036/#review88182

> I guess nukeSandbox is always a thing at this point? Can it throw or is that not a concern?

Yeah. This is ancient code from the time when the SDK was bundled with every add-on that used it, rather than included in Firefox.

Comment 11

[Kris Maglione [:kmag]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30dc9a187102dc4420ad405c16a632b494ed77cc

Comment 12

[Eric Rahm [:erahm]]

Looks like we're good!

> 4 WARNING: NS_ENSURE_TRUE(IsSandbox(sb)) failed: file js/xpconnect/src/XPCComponents.cpp, line 3024

This warning [1] shows up in the following test suites:

>      4 - desktop-test-linux64/debug-mochitest-devtools-chrome-9 dt9

It shows up in 4 tests. A few of the most prevalent:

>      1 -        devtools/client/debugger/test/mochitest/browser_dbg_addon-workers-dbg-enabled.js
>      1 -        devtools/client/debugger/test/mochitest/browser_dbg_addon-panels.js
>      1 -        devtools/client/debugger/test/mochitest/browser_dbg_addonactor.js
>      1 -        devtools/client/debugger/test/mochitest/browser_dbg_addon-sources.js

[1] https://hg.mozilla.org/try/annotate/30dc9a187102/js/xpconnect/src/XPCComponents.cpp#l3024

Comment 13

[Kris Maglione [:kmag]]

Ick. It looks like those tests bundle pre-built XPIs with ancient versions of the old style bootstrap.js loader. I'm not going to worry about them for now. They should be updated to use the new bootstrap code at some point, though.

Comment 14

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2d15e4f33d8a643197563608d1c216691abb18
Bug 1313440: Fix NS_ENSURE_TRUE(isSandbox) warnings in test code. r=erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/94d5d14da6bac6f013d6cc1187ac218092cea05e
Bug 1313440: Fix NS_ENSURE_TRUE(isSandbox) warnings in JPM bootstrap. r=erahm

Comment 15

[Kris Maglione [:kmag]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/353c47fc71ccff8b243c66df027350fe30dbce61
https://hg.mozilla.org/releases/mozilla-beta/rev/2eafbd4df272f9edb5160377530512add9370378

Only uplifting the test-only parts. The bootstrap.js that's part of the build is used by very few tests, so it shouldn't be a major issue.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb2d15e4f33d
https://hg.mozilla.org/mozilla-central/rev/94d5d14da6ba