Closed
Bug 1263629
Opened 8 years ago
Closed 8 years ago
Debugger steps into debugger internals in the browser toolbox
Categories
(DevTools :: Debugger, defect, P2)
DevTools
Debugger
Tracking
(firefox48+ fixed)
RESOLVED
FIXED
Firefox 48
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
Attachments
(1 file, 2 obsolete files)
6.22 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
That's because its promise module is Promise.jsm, which isn't flagged as "invisible to debugger". So the debugger consider it as something it should step into. Here is one STR from :jdescottes 1. open the rule view 2. open the browser toolbox 3. add a breakpoint in rules.js line 465 (that's "let nodeName = target && target.nodeName;" on Nightly) 4. back to Firefox, highlight some text in the ruleview, right click, copy 5. you should hit your breakpoint 6. step over
Assignee | ||
Comment 1•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=31640a1eefb3
Assignee | ||
Comment 2•8 years ago
|
||
I couldn't use Object.create() to make copy of paths and modules... so it is unecessary complex given that we have to ensure staying lazy for modules. I think it is fine hooking on `invisibleToDebugger` as it sounds very related!
Attachment #8739993 -
Flags: review?(jryans)
Assignee | ||
Comment 3•8 years ago
|
||
Waiting for green try before r?, but feedback is welcomed. May be I should start factoring code between all browser toolbox related tests?
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8739994 [details] [diff] [review] Test the browser toolbox debugger - v1 Review of attachment 8739994 [details] [diff] [review]: ----------------------------------------------------------------- Btw, this test is going to assert *NOT STEPPING INTO PROMISE WORLD*! Hopefully with this limited set of tests against the browser toolbox, we will soon stop regressing it.
Assignee | ||
Comment 5•8 years ago
|
||
Also note that this patch should speed up the browser toolbox a bit. It should prevent displaying tons of "would run" warning message in stdout and the console.
Comment on attachment 8739993 [details] [diff] [review] Use promise module invisible to the debugger for the browser toolbox server - v1 Review of attachment 8739993 [details] [diff] [review]: ----------------------------------------------------------------- Overall it seems reasonable. Are you willing to add test to verify the shared promise module is used when expected (to check on the promise rejection handling)? I think a test to just ensure the devtools loader gives the same promise instance as Cu.import of Promise.jsm would be sufficient. ::: devtools/shared/Loader.jsm @@ +116,5 @@ > + paths[path] = loaderPaths[path]; > + } > + let modules = {}; > + for (let name in loaderModules) { > + XPCOMUtils.defineLazyGetter(modules, name, (function (name) { Why are you using lazy getters here? It seems unrelated?
Attachment #8739993 -
Flags: review?(jryans) → feedback+
Did you actually experience the issue of stepping into the land of promises? And does this actually fix it for you?
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6) > Are you willing to add test to verify the shared promise module is used when > expected (to check on the promise rejection handling)? I think a test to > just ensure the devtools loader gives the same promise instance as Cu.import > of Promise.jsm would be sufficient. I can give that a try. > ::: devtools/shared/Loader.jsm > @@ +116,5 @@ > > + paths[path] = loaderPaths[path]; > > + } > > + let modules = {}; > > + for (let name in loaderModules) { > > + XPCOMUtils.defineLazyGetter(modules, name, (function (name) { > > Why are you using lazy getters here? It seems unrelated? Otherwise we making all the lazy defined attributes like xpcinspector, debugger, indexedDB non-lazy. But these lazy getters are still an issue. I have to change that, I don't know how yet. Loader.jsm has the same issue than Promise.jsm. It isn't invisible to debugger. So we get these "would run" warning messages and we possibly can step into the debugger dependencies world. (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7) > Did you actually experience the issue of stepping into the land of promises? > And does this actually fix it for you? Yes. Thanks to julian STR in comment 0.
[Tracking Requested - why for this release]: Let's make sure we land this in 48, since that's when we changed the promise modules in bug 1240804, which likely introduced this issue.
Assignee | ||
Comment 10•8 years ago
|
||
Add a test for checking promise module in regular and browser usecases. I'll followup with two things: - Get rid of the new and others "would run" warnings. It is important. It is exactly like promises. It means we are used non-invisible-to-debugger code when debugger is paused. It highlights cases where we could step into debugger internals :/ For example the lazy getter lives in Loader.jsm which is not invisible to debugger. We should fix that. - Get the browser toolbox debugger test green. That looks challenging and I don't want to block promise fix on that. To get back to this patch. One possible improvement could be to move paths and modules definition into BuiltinProvider.load so that we do not have to copy the dictionnary nor have to do double lazy getters...
Attachment #8741344 -
Flags: review?(jryans)
Assignee | ||
Updated•8 years ago
|
Attachment #8739993 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
FYI, I'm fixing another "would run" warning in bug 1263935.
Assignee | ||
Updated•8 years ago
|
Attachment #8739994 -
Attachment is obsolete: true
Updated•8 years ago
|
Priority: -- → P2
Comment on attachment 8741344 [details] [diff] [review] Use promise module invisible to the debugger for the browser toolbox server - v2 Review of attachment 8741344 [details] [diff] [review]: ----------------------------------------------------------------- Great! Let's land this soon! :D
Attachment #8741344 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab56f2e37d52
Assignee | ||
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/6ddb8429d92077d93cb705f5758d75e028fac86c Bug 1263629 - Use promise module invisible to the debugger for the browser toolbox server. r=jryans
Blocks: 1264900
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6ddb8429d920
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•