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)

defect

Tracking

(firefox48+ fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 + fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 2 obsolete files)

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
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)
Waiting for green try before r?, but feedback is welcomed.
May be I should start factoring code between all browser toolbox related tests?
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.
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?
(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.
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)
Attachment #8739993 - Attachment is obsolete: true
FYI, I'm fixing another "would run" warning in bug 1263935.
Depends on: 1264625
Depends on: 1264626
Attachment #8739994 - Attachment is obsolete: true
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+
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
https://hg.mozilla.org/mozilla-central/rev/6ddb8429d920
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Recent regression, tracking in case it reopens.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.