Closed Bug 1189694 Opened 5 years ago Closed 5 years ago

Only consider Debugger.Sources we haven't seen before

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Attachment #8641599 - Flags: review?(ttromey)
Under what circumstances would addSource be called for a Debugger.Source that we've already seen before?
I think it might be _restoreBreakpoints' fault, which calls addSource for every script's source in the script store, but if it is in the script store, then we must have already added the source and at which time addSource would have restored the source's breakpoints. I think that method is vestigial from before the script store and can perhaps be removed.
Ok, so I think the frontend relies on newSource packets coming in fairly early (which it totally shouldn't rely on) and I'm getting mochitest failures despite the xpcshell suite working fine.

Going to remove the newSource packet delaying because it isn't worth the headache.
Attachment #8641599 - Attachment is obsolete: true
Attachment #8641599 - Flags: review?(ttromey)
Summary: Only consider Debugger.Sources we haven't seen before and delay sending newSource RDP packets until after higher priority tasks → Only consider Debugger.Sources we haven't seen before
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #4)
> Ok, so I think the frontend relies on newSource packets coming in fairly
> early (which it totally shouldn't rely on) and I'm getting mochitest
> failures despite the xpcshell suite working fine.
> 
> Going to remove the newSource packet delaying because it isn't worth the
> headache.
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #3)
> I think it might be _restoreBreakpoints' fault, which calls addSource for
> every script's source in the script store, but if it is in the script store,
> then we must have already added the source and at which time addSource would
> have restored the source's breakpoints. I think that method is vestigial
> from before the script store and can perhaps be removed.

That makes sense. Instead of maintaining a set of D.S. instances that we've already seen, could we simply remove this method? That seems like a cleaner solution.
Hrm. I'm still getting a bunch of duplicate D.Sources from onNewScript, so it wasn't just _restoreBreakpoints...

Looking into it.
Comment on attachment 8641854 [details] [diff] [review]
Only consider Debugger.Sources we haven't seen before

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

Still looking into the root cause, but I think we can land this in the meantime since the perf degradation is so severe. Two possibilities are eval cache being buggy and/or script cloning into new compartments.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=046d3f13b229
Attachment #8641854 - Flags: review?(jsantell)
Function cloning indeed fires the onNewScript hook with functions that are not top level or direct evals.

(gdb) bt 10
#0  js::Debugger::fireNewScript (this=0x12cc71000, cx=0x132658c00, script={<js::HandleBase<JSScript *>> = {<No data fields>}, ptr = 0x7fff5fbccb48}) at Debugger.cpp:1247
#1  0x0000000107e6d8a5 in js::Debugger::slowPathOnNewScript(JSContext*, JS::Handle<JSScript*>)::$_7::operator() (this=0x7fff5fbcc988, dbg=0x12cc71000) at Debugger.cpp:1415
#2  0x0000000107e1ddb3 in js::Debugger::dispatchHook<js::Debugger::slowPathOnNewScript(JSContext*, JS::Handle<JSScript*>)::$_6, js::Debugger::slowPathOnNewScript(JSContext*, JS::Handle<JSScript*>)::$_7> (cx=0x132658c00, hookIsEnabled={script = {<js::HandleBase<JSScript *>> = {<No data fields>}, ptr = 0x7fff5fbccb48}}, fireHook={cx = @0x7fff5fbcca70, script = @0x7fff5fbcca78}) at Debugger.cpp:1398
#3  0x0000000107e1da7d in js::Debugger::slowPathOnNewScript (cx=0x132658c00, script={<js::HandleBase<JSScript *>> = {<No data fields>}, ptr = 0x7fff5fbccb48}) at Debugger.cpp:1409
#4  0x0000000107ea23d9 in js::Debugger::onNewScript (cx=0x132658c00, script={<js::HandleBase<JSScript *>> = {<No data fields>}, ptr = 0x7fff5fbccb48}) at Debugger.h:1057
#5  0x00000001085e7661 in js::CloneFunctionAndScript (cx=0x132658c00, fun={<js::HandleBase<JSFunction *>> = {<No data fields>}, ptr = 0x7fff5fbcfa68}, parent={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x12a48c0e0}, newStaticScope={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbccd00}, allocKind=js::gc::FIRST, proto={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x108d01590}) at jsfun.cpp:2193
#6  0x0000000107ea5a2e in js::CloneFunctionObjectIfNotSingleton (cx=0x132658c00, fun={<js::HandleBase<JSFunction *>> = {<No data fields>}, ptr = 0x7fff5fbcfa68}, parent={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x12a48c0e0}, proto={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x108d01590}, newKind=js::GenericObject) at jsfuninlines.h:96
#7  0x0000000107e6033d in js::Lambda (cx=0x132658c00, fun={<js::HandleBase<JSFunction *>> = {<No data fields>}, ptr = 0x7fff5fbcfa68}, parent={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x12a48c0e0}) at Interpreter.cpp:4282
#8  0x0000000107e558ce in Interpret (cx=0x132658c00, state=@0x7fff5fbcfea0) at Interpreter.cpp:3530
#9  0x0000000107e42622 in js::RunScript (cx=0x132658c00, state=@0x7fff5fbcfea0) at Interpreter.cpp:752
Ok this is a mess. Just spent way too much of my life on this. Basically, hacking around the lack of a onNewSource hook (which is what we've really always wanted and used onNewScript for) is a lot harder in the platform than in the actor.

We have to fire the hook for cloned functions so that you can get a chance to set a BP on them, but they will never be static level = 0, and in order to avoid repeatedly firing the hook we would need to keep a set of sources whose scripts have been fired in onNewScript, but that is easier to do in JS, and is actually exactly what the already attached patch does.
Attachment #8641854 - Flags: review?(jsantell) → review+
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #10)
> Ok this is a mess. Just spent way too much of my life on this. Basically,
> hacking around the lack of a onNewSource hook (which is what we've really
> always wanted and used onNewScript for) is a lot harder in the platform than
> in the actor.
> 
> We have to fire the hook for cloned functions so that you can get a chance
> to set a BP on them, but they will never be static level = 0, and in order
> to avoid repeatedly firing the hook we would need to keep a set of sources
> whose scripts have been fired in onNewScript, but that is easier to do in
> JS, and is actually exactly what the already attached patch does.

I see that the patch already landed, but this could definitely use a comment to that effect.
https://hg.mozilla.org/mozilla-central/rev/61ab62f2fa33
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.