1617210 - Properties with same values can't be expanded in the scopes panel

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Sören Hentzschel]

Attachment: screencast

Posting this on behalf of another user in a Firefox support forum.

When using the debugger in the browser toolbox you can't expand the "target" property in the right sidebar. I don't know if other properties are also affected and if it's also an issue with the web debugger but I got the attached screencast which shows the issue better than I am able to explain it (I've never used the debugger).

According to mozregression it's a regression from bug 1567849.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Hello Sören, thanks for reporting the issue. The regression looks odd since the bug only modified methods used by the console client.
It looks like you're testing the Browser Toolbox? Could you tell me in which file, and which line you put the breakpoint? Thanks!

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

I'm able to reproduce with data:text/html,<meta charset=utf8><script>document.addEventListener("click", function(e) {debugger;})</script>

Comment 3

[Sören Hentzschel]

The user in the support forum gave me chrome:// -> content -> elements > arrowscrollbox.js line 822 as an example but I can confirm that your example also reproduces the issue.

I don't know if other properties are also affected and if it's also an issue with the web debugger

Thanks to your example I can answer these questions: Other (but not all) properties are also affected and it also happens with the web debugger, not only the browser toolbox. I changed the bug title to reflect that not only the "target" property is affected.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

using mozregression the pushlog can't get smaller than https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3c08edf74d039af79f9daad8ff5b57ffb64fdab6&tochange=32f84899fc000d95337a25c14a27470d0c52e3ee

My guess would be more Bug 1579090 , we need to investigate what's going on in the scopes panel.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

okay I managed to have a simpler STR:

1. Navigate to data:text/html,<script>var hello = {hello: "world"}; x = {a: hello, b: hello}; debugger;</script>
2. Open the debugger
3. Reload so we hit the debugger statement
4. Expand window and then x, and then try to expand a

Actual result: a can't be expanded.
The reason is that we create 2 fronts for the properties of x, one for a, one for b.
But they refer to the same actor which confuses protocol.js. Basically, when we call enumProperties on Front "A", that's Front "B" that will try to handle the response. And since the request didn't initially came from "B", we don't have the callback we should execute.

This only impacts the debugger because in the thread actor we're caching the object actors (devtools/server/actors/thread.js#1576-1578). In the console we get 2 different actors for a and b, so we don't hit this issue.

I'm tempted to remove the cache mechanism has a hotfix that could be uplifted all the way to release, and then think about a proper fix (that mayb be: don't cache those actors).

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

I have a much better and simpler approach, will post a patch as soon as I have a test written.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1617210 - Fix issue with duplicated ObjectFronts in the scopes panel. r=jlast.

Since the thread actor caches the object actors
it creates, if an object had multiple properties
referencing the same object, we would create
multiple fronts for the same object actor, which
would confuse protocol.js.
The fix consist in checking if a front already exists
before creating a new one.
A test is added for the debugger to ensure this
works as expected and we don't regress.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/336f88eaf4e2
Fix issue with duplicated ObjectFronts in the scopes panel. r=jlast.

Comment 9

[Andreea Pavel [:apavel]]

Backed out for causing bug 1617812 and xpcshell failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=336f88eaf4e2ca24bd8eec46d66d093b9c4fadcd&selectedJob=290315283

Failure log:

• node: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=290315283&repo=autoland&lineNumber=662
• xpcshell: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=290316712&repo=autoland&lineNumber=2932

Backout: https://hg.mozilla.org/integration/autoland/rev/a2c0deaf95996cfa034c37075b902e97cd546185

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

pushed a fix version to TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b55a57a9bdb59c53a09a3d2f40c33188e6c53a00

• mocha tests were failing because we didn't had getFrontByID in our conn mock
• xpcshell test was failing because we weren't releasing object front, and I guess we were getting another actor with the same id, which means we were reusing the same ObjectFront (which was outdated).

Comment 11

[Andreea Pavel [:apavel]]

Nicolas this also caused some dt failures https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=9f62d77fd22876a37aed707ebf89e3d533a30017&selectedJob=290319158

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=290319158&repo=autoland&lineNumber=25531

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

yes, this is the same root cause as the node failure, my last push to TRY looks good so I'll land that again :)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef3886a28cbc
Fix issue with duplicated ObjectFronts in the scopes panel. r=jlast.

Comment 14

[Andreea Pavel [:apavel]]

Thanks!

Comment 15

[Natalia Csoregi [:nataliaCs]]

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

Comment 16

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

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 17

[Pascal Chevrel:pascalc]

Nicolas, do you think that this is material we should uplift in our last 74 beta or can it ride the 75 train? Thanks

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

ideally i would have like this to be uplifted all the way up to release, but maybe it's too late?
I'm going to request the beta uplift for now

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9128551 [details]
Bug 1617210 - Fix issue with duplicated ObjectFronts in the scopes panel. r=jlast.

Beta/Release Uplift Approval Request

• User impact if declined: User can't expand a property in debugger's scopes panel if there's multiple properties with the same value object
• 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: 1. Navigate to data:text/html,<script>var hello = {hello: "world"}; x = {a: hello, b: hello}; debugger;</script>

1. Open the debugger
2. Reload so the debugger statement is hit and the debugger pauses
3. In the scopes panel, expand window and then x, and then try to expand a

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): a couple of lines, DevTools only fix, with thorough automated test
• String changes made/needed:

Comment 20

[Alin Ilea, Desktop Test Engineering]

Reproduced the initial issue using Release version 73.0.1 using Windows 10.
Verified - Fixed in latest Nightly build 75.0a1 (Build id: 20200225214332) using Windows 10, Mac OS 10.14 and Ubuntu 18.04.

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

Pascal, let's hold on the uplift for now until I investigate the performance regression caused by this

Comment 22

[Pascal Chevrel:pascalc]

Ok sounds good

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Too late for 73 with 74 going to RC next week.

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

Pascal, let's see if we can uplift this to beta as it's pretty important and we prefer to be correct and slow than incorrect and fast

Comment 25

[Pascal Chevrel:pascalc]

Comment on attachment 9128551 [details]
Bug 1617210 - Fix issue with duplicated ObjectFronts in the scopes panel. r=jlast.

Uplift approved for 74 beta 9, thanks.

Comment 26

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

https://hg.mozilla.org/releases/mozilla-beta/rev/554069d79cd6

Comment 27

[Alin Ilea, Desktop Test Engineering]

Verified - Fixed in latest Beta build 74.0b9 (Build id: 20200227210932) using Windows 10, Mac OS 10.14 and Ubuntu 18.04.