Closed Bug 1131756 Opened 9 years ago Closed 9 years ago

Debugger doesn't break in react hot reload demo

Categories

(DevTools :: Debugger, defect, P1)

36 Branch
x86
macOS
defect

Tracking

(firefox40 fixed, firefox41 fixed, relnote-firefox -)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed
relnote-firefox --- -

People

(Reporter: canuckistani, Assigned: jlong)

References

(Blocks 1 open bug)

Details

(Whiteboard: [polish-backlog])

Attachments

(3 files, 3 obsolete files)

In the react hotreload demo the user is told to un-comment a line in the editor that should cause the debugger to pause:

    // Feel like debugging something?
    // Open DevTools and uncomment next line:
    console.log(e); debugger;
    // Now type something into the input.
    // Nice, huh? Remove this line so we can move on.

see http://gaearon.github.io/react-hot-loader/
Assignee: nobody → jlong
Whiteboard: [devedition-40]
It does break in nightly, but the editor doesn't show the right source and switching through the stack frames is messed up. This is because we hide eval sources that don't have a URL. I'm going to see how easy it is to hide them, but make one appear if the editor needs it.
Attached patch 1131756.patch (obsolete) — Splinter Review
Needs tests, so not quite finished yet, but it works: https://twitter.com/jlongster/status/593155967317102592
Priority: -- → P1
Attached patch 1131756.patch (obsolete) — Splinter Review
This patch makes it so that if we break inside a script that is not listed in the sources pane (happens when a `debugger` statement is inside an anonymous eval), we add it to the source pane so we can interact with it. Doing so makes `debugger` statements work properly; the eval script is shown.

This reminds me that we need to implement functionality for jumping to the introduction site at some point
Attachment #8598927 - Attachment is obsolete: true
Attachment #8599462 - Flags: review?(ejpbruel)
Comment on attachment 8599462 [details] [diff] [review]
1131756.patch

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

Patch looks good to me.

::: browser/devtools/debugger/debugger-controller.js
@@ +689,5 @@
>      for (let frame of this.activeThread.cachedFrames) {
>        let { depth, source, where: { line } } = frame;
>  
>        let isBlackBoxed = source ? this.activeThread.source(source).isBlackBoxed : false;
> +      DebuggerView.StackFrames.addFrame(frame, line, depth, isBlackBoxed);

This is much cleaner. I like it.

::: browser/devtools/debugger/views/sources-view.js
@@ +121,5 @@
>      this._copyUrlMenuItem.removeEventListener("command", this._onCopyUrlCommand, false);
>      this._newTabMenuItem.removeEventListener("command", this._onNewTabCommand, false);
>    },
>  
> +  empty: function() {

You don't seem to be calling this method explicitly anywhere. Is it somehow called implicitly by WidgetMethods?
Attachment #8599462 - Flags: review?(ejpbruel) → review+
(In reply to Eddy Bruel [:ejpbruel] from comment #6)\
> 
> ::: browser/devtools/debugger/views/sources-view.js
> @@ +121,5 @@
> >      this._copyUrlMenuItem.removeEventListener("command", this._onCopyUrlCommand, false);
> >      this._newTabMenuItem.removeEventListener("command", this._onNewTabCommand, false);
> >    },
> >  
> > +  empty: function() {
> 
> You don't seem to be calling this method explicitly anywhere. Is it somehow
> called implicitly by WidgetMethods?

It is called explicitly in various places, like in `handleTabNavigation` in debugger-view.js. It is called whenever we want to clear out the sources.
Attached patch 1131756.patch (obsolete) — Splinter Review
new patch to fix a test failure
Attachment #8599462 - Attachment is obsolete: true
There are some strange failures in there but I don't think any of them are related to this:

* everything has the "menu.getItemByValue(...) is undefined" messages, even green runs so that must be something failing in a promise that we need to fix (but unrelated, coming from netmonitor)

* on linux debug, tests are timing out in various ways

* on windows 7 opt, tilt tests are failing
Keywords: checkin-needed
(In reply to James Long (:jlongster) from comment #10)
> There are some strange failures in there but I don't think any of them are
> related to this:
> 
> * everything has the "menu.getItemByValue(...) is undefined" messages, even
> green runs so that must be something failing in a promise that we need to
> fix (but unrelated, coming from netmonitor)

Filed Bug 1162961 for that
We need to make sure to land this today for devedition-40.
(In reply to James Long (:jlongster) from comment #12)
> We need to make sure to land this today for devedition-40.

We can always request uplift - we just need to make sure it's in by 40.1.
(In reply to Brian Grinstead [:bgrins] from comment #13)
> (In reply to James Long (:jlongster) from comment #12)
> > We need to make sure to land this today for devedition-40.
> 
> We can always request uplift - we just need to make sure it's in by 40.1.

This adds new l10n strings though, and as far as I'm aware those are a lot harder (impossible?) to uplift. Worst case it could be unlocalized I guess, but still.
(In reply to James Long (:jlongster) from comment #14)
> (In reply to Brian Grinstead [:bgrins] from comment #13)
> > (In reply to James Long (:jlongster) from comment #12)
> > > We need to make sure to land this today for devedition-40.
> > 
> > We can always request uplift - we just need to make sure it's in by 40.1.
> 
> This adds new l10n strings though, and as far as I'm aware those are a lot
> harder (impossible?) to uplift. Worst case it could be unlocalized I guess,
> but still.

Oh yeah, we would need to get strings in today.  Ryan, is it going to be possible to land this before the merge?  Trees are closed, and I'm not sure when it needs to be in m-c to make the release.
Flags: needinfo?(ryanvm)
Status: NEW → ASSIGNED
If you want to land just the strings on m-c, a=me (with a DONTBUILD in the commit message to avoid useless builds/tests). Otherwise, sorry, this is the risk you take by not getting landed by the Friday before the uplift.
Flags: needinfo?(ryanvm)
James, can you break out the strings so we can push them to m-c?  You'll need to keep evalGroupLabel in the file and add anonymousSourcesLabel alongside it since the strings are going to have to ship separate from the patch.

Then in the new version of the full patch you can remove evalGroupLabel
Flags: needinfo?(jlong)
Alright, here are just the strings to be landed before uplift. Sorry about that.
Flags: needinfo?(jlong)
The other patch will need a rebase now for the string files
Attached patch 1131756.patchSplinter Review
rebased
Attachment #8602923 - Attachment is obsolete: true
Pushing a new try because the last one had weird unrelated failures and I'm hoping I can get a green run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39f60dd0a4aa
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: debugger doesn't break correctly in eval scripts
[Describe test coverage new/current, TreeHerder]: on m-c
[Risks and why]: not much risk, only in the debugger devtool
[String/UUID change made/needed]:
Attachment #8606367 - Flags: approval-mozilla-aurora?
Target Milestone: --- → Firefox 41
Can we get the uplift approved soon? This needs to sit on aurora for a little bit before June 2
Flags: needinfo?(lhenry)
sorry for the spam, I think this is the right contact
Flags: needinfo?(lhenry) → needinfo?(lmandel)
Comment on attachment 8606367 [details] [diff] [review]
1131756-aurora.patch

Approved for uplift to aurora; this has been stable on m-c for a few days. Looks like it should improve dev tools, so it will be good to have this on aurora.
Attachment #8606367 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Oops, I didn't notice that it put text in this field.

Release Note Request (optional, but appreciated)
[Why is this notable]: Developers debugging unnamed eval scripts will notice that they can put `debugger` statements in them now and we will probably debug them
[Suggested wording]: Eval scripts without a `sourceURL` pragma can break on debugger statements
[Links (documentation, blog post, etc)]:
I'm not sure this is good for release notes but maybe good for the MDN page over here: https://developer.mozilla.org/en-US/Firefox/Releases/39
Flags: needinfo?(lmandel)
Whiteboard: [devedition-40] → [polish-backlog]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.