The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Developer Tools: Debugger
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
Firefox 17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
Created attachment 645793 [details] [diff] [review]
v1

Try push: https://tbpl.mozilla.org/?tree=Try&rev=83bdeee4cb79
Attachment #645793 - Flags: review?(rcampbell)
Attachment #645793 - Flags: review?(rcampbell) → review+
Status: NEW → ASSIGNED
Whiteboard: [land-in-fx-team]

Comment 2

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/8d3b2c84e7be
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8d3b2c84e7be
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 17
(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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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

5 years ago
Seems like it would be more clear to use a global var to store truly global state rather than a prototype property.
(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?
Created attachment 649350 [details] [diff] [review]
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.
Attachment #649350 - Flags: feedback?(past)
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.
Attachment #649350 - Flags: feedback?(past) → feedback+
(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?
(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.
Created attachment 649775 [details] [diff] [review]
Tests to ensure that the breakpoint store is shared
Attachment #649775 - Flags: review?(past)
Attachment #649350 - Flags: review?(past)
Attachment #649350 - Flags: review?(past) → review+
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.
Attachment #649775 - Flags: review?(past) → review+
Created attachment 650308 [details] [diff] [review]
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.
Attachment #649775 - Attachment is obsolete: true
Attachment #650308 - Flags: checkin?(past)
Attachment #649350 - Flags: checkin?(past)
(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.
Whiteboard: [fixed-in-fx-team]
Attachment #649350 - Flags: checkin?(past) → checkin+
Attachment #650308 - Flags: checkin?(past) → checkin+
filter on chocolate
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/c7bc29d4de78
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.