Last Comment Bug 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
: Move ThreadActor.prototype.{_scripts, _breakpointStore} initialization to the...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 17
Assigned To: Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-24 16:12 PDT by Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
Modified: 2012-08-09 11:50 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (1.12 KB, patch)
2012-07-25 09:30 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
rcampbell: review+
Details | Diff | Review
proposed regression fix (1.01 KB, patch)
2012-08-06 12:32 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
past: review+
past: feedback+
past: checkin+
Details | Diff | Review
Tests to ensure that the breakpoint store is shared (1.41 KB, patch)
2012-08-07 13:25 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
past: review+
Details | Diff | Review
Tests to ensure that the breakpoint store is shared (with comment) (1.52 KB, patch)
2012-08-08 14:18 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
past: checkin+
Details | Diff | Review

Description Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-07-24 16:12:45 PDT
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] [⏰PDT; UTC-7] 2012-07-25 09:30:41 PDT
Created attachment 645793 [details] [diff] [review]
v1

Try push: https://tbpl.mozilla.org/?tree=Try&rev=83bdeee4cb79
Comment 3 Tim Taubert [:ttaubert] 2012-07-31 07:40:52 PDT
https://hg.mozilla.org/mozilla-central/rev/8d3b2c84e7be
Comment 4 Panos Astithas [:past] 2012-08-01 04:00:09 PDT
(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] [⏰PDT; UTC-7] 2012-08-02 16:58:45 PDT
(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) 2012-08-02 23:43:43 PDT
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 [:past] 2012-08-03 03:49:32 PDT
(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] [⏰PDT; UTC-7] 2012-08-06 12:32:19 PDT
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.
Comment 9 Panos Astithas [:past] 2012-08-07 01:04:28 PDT
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] [⏰PDT; UTC-7] 2012-08-07 08:45:49 PDT
(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 [:past] 2012-08-07 10:27:42 PDT
(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] [⏰PDT; UTC-7] 2012-08-07 13:25:47 PDT
Created attachment 649775 [details] [diff] [review]
Tests to ensure that the breakpoint store is shared
Comment 13 Panos Astithas [:past] 2012-08-08 01:50:14 PDT
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] [⏰PDT; UTC-7] 2012-08-08 14:18:13 PDT
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.
Comment 15 Panos Astithas [:past] 2012-08-09 04:07:10 PDT
(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] [⏰PDT; UTC-7] 2012-08-09 09:21:30 PDT
filter on chocolate
Comment 17 Tim Taubert [:ttaubert] 2012-08-09 11:50:30 PDT
https://hg.mozilla.org/mozilla-central/rev/c7bc29d4de78

Note You need to log in before you can comment on or make changes to this bug.