777146 - Move ThreadActor.prototype.{_scripts, _breakpointStore} initialization to the constructor so that each instance of ThreadActor has its own {_scripts, _breakpointStore} object instead of each instance sharing the same object

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Dave raised the point that this could be causing some of the bugs we are seeing where scripts from other tabs are showing up in the debugger. Not sure which bug # that is though.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v1

Try push: https://tbpl.mozilla.org/?tree=Try&rev=83bdeee4cb79

Comment 2

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/8d3b2c84e7be

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8d3b2c84e7be

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Nick Fitzgerald :fitzgen from comment #0)
> Dave raised the point that this could be causing some of the bugs we are
> seeing where scripts from other tabs are showing up in the debugger. Not
> sure which bug # that is though.

I think Dave was referring to bug 710258 (before it was repurposed) and I believe he is right.

However, moving the breakpoint store to the constructor, regresses breakpoints on startup. For example, the STR in bug 754251 result in a breakpoint that no longer pauses after a reload. Had I been able to write a test for that patch, we would have been able to catch this regression (but see bug 754251 comment 3 for why that is not currently possible). Is there any specific reason for moving the breakpoint store along with the scripts to the constructor?

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #4)
> Is there any specific reason for moving the breakpoint store along with the
> scripts to the constructor?

No, it just looked like a bug as well since there was no comment describing why all instances of ThreadActor should share the same breakpoint store. Sorry >_<

Comment 6

[Dave Camp (:dcamp)]

Seems like it would be more clear to use a global var to store truly global state rather than a prototype property.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Dave Camp (:dcamp) from comment #6)
> Seems like it would be more clear to use a global var to store truly global
> state rather than a prototype property.

It's not really global, it's just that it needs to persist page reloads. If we had something resembling the behavior of an outer window, I'd put it as its property, but tab actors are recycled on navigation, too. I could put it as a property of the root actor, which is one step away from a global var, but this seems hacky, too.

If we ever decide to persist breakpoints across browser sessions, a global would make perfect sense, but this doesn't seem good UX to me and IIRC no other browsers do it either. 

Thoughts?

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: proposed regression fix

Put |_breakpointStore| on |ThreadActor| because it is more of a class member rather than an instance member. Added a getter to the prototype so that a) I didn't have to change as much code b) no |ThreadActor| instance can accidentally override the |_breakpointStore| property and have a different set of breakpoints from the other |ThreadActor| instances.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 649350 [details] [diff] [review]
proposed regression fix

Review of attachment 649350 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see much benefit over just sticking the breakpoint store back to the prototype, but it doesn't bother me either.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #9)
> Comment on attachment 649350 [details] [diff] [review]
> proposed regression fix
> 
> Review of attachment 649350 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see much benefit over just sticking the breakpoint store back to the
> prototype, but it doesn't bother me either.

To relate to more classically OO languages, I think that |Foo.prototype.bar| would be considered an instance member and |Foo.bar| would be a class member.

Of course, this implementation ends up being equivalent to how it was before, but I think that the intent of the code is clearer this way.

What do we need for this to land? Tests? Is writing a test for this case still impossible?

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Nick Fitzgerald :fitzgen from comment #10)
> (In reply to Panos Astithas [:past] from comment #9)
> > Comment on attachment 649350 [details] [diff] [review]
> > proposed regression fix
> > 
> > Review of attachment 649350 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I don't see much benefit over just sticking the breakpoint store back to the
> > prototype, but it doesn't bother me either.
> 
> To relate to more classically OO languages, I think that |Foo.prototype.bar|
> would be considered an instance member and |Foo.bar| would be a class member.
> 
> Of course, this implementation ends up being equivalent to how it was
> before, but I think that the intent of the code is clearer this way.
> 
> What do we need for this to land? Tests? Is writing a test for this case
> still impossible?

I suppose you could write a contrived xpcshell test that makes sure the store is in the class. That should suffice I think.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Tests to ensure that the breakpoint store is shared

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 649775 [details] [diff] [review]
Tests to ensure that the breakpoint store is shared

Review of attachment 649775 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine. Just add a comment in the test to note that removing this would regress bug 754251, in case anyone tries to remove it in the future.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Tests to ensure that the breakpoint store is shared (with comment)

Added the requested comment.

I can only make user repos, not push to fx-team/m-c/m-i/etc, could you please commit the fix and this test for me Panos? Thanks.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Nick Fitzgerald :fitzgen from comment #14)
> I can only make user repos, not push to fx-team/m-c/m-i/etc, could you
> please commit the fix and this test for me Panos? Thanks.

Done:
https://hg.mozilla.org/integration/fx-team/rev/c7bc29d4de78

In the future just add [land-in-fx-team] in the whiteboard and someone will take care of it.

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

filter on chocolate

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

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