1544175 - Inspecting CPOW crashes tab

Status: VERIFIED FIXED

(DevTools :: Console, defect, P3)

Description

[Oriol Brufau [:Oriol]]

1. Enable devtools.chrome.enabled and devtools.debugger.remote-enabled
2. In the browser console, run

   var listener = function(message) { console.log(message.objects) };
   Services.ppmm.addMessageListener("foo", listener);
   

3. In the browser content toolbox, run

   var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
   Services.cpmm.sendAsyncMessage("foo", null, {foo: {a:1}});
   

4. You will see an object in the browser console
5. If the tab has not crashed yet, expand that object

The tab crashes: https://crash-stats.mozilla.org/report/index/2ae75818-b24a-4d64-a390-381020190413

According to bug 1465911 comment 6, the crash seems deliberate.
It's just that devtools should avoid accessing CPOWs.

In https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/devtools/client/webconsole/test/mochitest/browser_console.js#115 there is a test about CPOWs, but it's no longer checking anything because it relies on _contentWindow, which has been removed.

In object.js there is some logic to detect CPOWs but it assumes that DevToolsUtils.isSafeDebuggerObject will return false for them because they are invisible to debugger, see bug 1391449. However, this does no longer seem to be the case. Now they can be unwrapped, but just trying to access the class crashes the tab.

Comment 1

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

Closing because no crashes reported for 12 weeks.

Comment 2

[Oriol Brufau [:Oriol]]

Here is your crash report: https://crash-stats.mozilla.org/report/index/7c00bf43-9fd3-4f70-983b-381ea0190717

Comment 3

[Oriol Brufau [:Oriol]]

Attachment: Bug 1544175 - Prevent devtools from accessing CPOWs. r?nchevobbe

Comment 4

[Oriol Brufau [:Oriol]]

Attachment: Bug 1544175 - Fix browser_console.js. r?nchevobbe

Depends on D38803

Comment 5

[Oriol Brufau [:Oriol]]

(In reply to Oriol Brufau [:Oriol] from comment #0)

there is a test about CPOWs, but it's no longer checking anything because it relies on _contentWindow, which has been removed.

Actually the removal of _contentWindow should have made the test fail, but browser_console.js is not checking anything at all. Using content outside of ContentTask.spawn throws an error at the beginning, so the test ends prematurely. But it doesn't fail due to a badly placed expectUncaughtException(). I included a fix for this while I was at it.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c2a0bf402ac
Prevent devtools from accessing CPOWs. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/ee04e52a4ddf
Fix browser_console.js. r=nchevobbe

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3c2a0bf402ac
https://hg.mozilla.org/mozilla-central/rev/ee04e52a4ddf

Comment 8

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

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta for Fx69 or can it ride to release with Fx70?

Comment 10

[Oriol Brufau [:Oriol]]

Yes I would like to uplift this. But if bug 1567726 is not uplifted first, I will have to add toolbox.selectTool("jsdebugger"); into the test to avoid a failure.

Comment 11

[Oriol Brufau [:Oriol]]

Attachment: D38803-for-beta.diff

Beta/Release Uplift Approval Request

• User impact if declined: Inspecting CPOW in devtools will crash tab
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just makes sure that devtools don't access CPOWs.
• String changes made/needed:

Comment 12

[Oriol Brufau [:Oriol]]

For the beta patch I fixed a conflict in browser.ini and in browser_console_cpow.js I added a workaround for bug 1567726

Comment 13

[Bogdan Maris, Desktop QA]

Reproduced the initial crash using an old Nightly build: https://crash-stats.mozilla.org/report/index/aeabd9be-2647-42a6-978a-bacfa0190802
Verified that the crash does not occur using latest Nightly 70.0a1 on macOS 10.13.6, Ubuntu 18.04 64bit and Windows 10 64bit.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

I guess we're waiting on an answer to the NI in bug 1567726 before taking this.

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Oriol Brufau [:Oriol] from comment #12)

For the beta patch I fixed a conflict in browser.ini and in browser_console_cpow.js I added a workaround for bug 1567726

Oriol, we are not going to uplift bug 1567726, is the patch you attached to this bug ready for uplift without 1567726 or should it be updated? Thanks

Comment 16

[Oriol Brufau [:Oriol]]

Yes, the beta patch has the workaround for bug 1567726

Comment 17

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9082417 [details] [diff] [review]
D38803-for-beta.diff

Uplift approved for 69 beta 12, thanks.

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/605fe408fac1

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out to reland with approver in commit message: https://hg.mozilla.org/releases/mozilla-beta/rev/60b277d0ef161532e1b25601d23f1d3bbca47e98

Reland: https://hg.mozilla.org/releases/mozilla-beta/rev/22553e8e87c9

Comment 20

[Bogdan Maris, Desktop QA]

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #13)

Reproduced the initial crash using an old Nightly build: https://crash-stats.mozilla.org/report/index/aeabd9be-2647-42a6-978a-bacfa0190802
Verified that the crash does not occur using latest Nightly 70.0a1 on macOS 10.13.6, Ubuntu 18.04 64bit and Windows 10 64bit.

Also verified using 69.0b12 on all platforms (macOS 10.13.6, Ubuntu 18.04 64bit and Windows 10 64bit).