Closed
Bug 1313440
Opened 8 years ago
Closed 8 years ago
2,900 instances of "NS_ENSURE_TRUE(IsSandbox(sb)) failed" emitted from js/xpconnect/src/XPCComponents.cpp during linux64 debug testing
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: erahm, Assigned: kmag)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
> 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•8 years ago
|
||
That seems to suggest that the jetpack code passes something that's not a sandbox to nukeSandbox, which is probably a bug.
Comment 2•8 years ago
|
||
Regression from bug 1309351?
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee: nobody → kmaglione+bmo
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox52:
--- → affected
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
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.
Reporter | ||
Comment 7•8 years ago
|
||
mozreview-review |
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?
Reporter | ||
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8805257 [details] Bug 1313440: Fix NS_ENSURE_TRUE(isSandbox) warnings in test code. https://reviewboard.mozilla.org/r/89036/#review88184
Attachment #8805257 -
Flags: review?(erahm) → review+
Reporter | ||
Comment 9•8 years ago
|
||
mozreview-review |
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.
Attachment #8805258 -
Flags: review?(erahm) → review+
Assignee | ||
Comment 10•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=30dc9a187102dc4420ad405c16a632b494ed77cc
Reporter | ||
Comment 12•8 years ago
|
||
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
Assignee | ||
Comment 13•8 years ago
|
||
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.
Assignee | ||
Comment 14•8 years ago
|
||
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
Assignee | ||
Comment 15•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fb2d15e4f33d https://hg.mozilla.org/mozilla-central/rev/94d5d14da6ba
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•