Correction: even proper breakpoints set in devtools UI disrupt API initialization when debugging with devtools open.

The extension is attached below.

Deleted: test-debugger.zip 1.0 KB

test-debugger.zip 1.0 KB Download

Able to reproduce the issue on Windows-7,Windows-10,Mac-10.12.4 and Linux Ubuntu-14.04 using chrome stable version # 60.0.3112.90  and canary 62.0.3182.0 with the steps mentioned in comment#0

As per comment #0 
Good Build Revision: 423547 
Bad Build Revision : 423558

CHANGE-LOG URL:
---------------
https://chromium.googlesource.com/chromium/src/+/a9bc8ccd2becc952e4cea2d8a053247a481dd483

Suspecting https://codereview.chromium.org/2339683006 from changelog

@rdevlin.cronin: Could you please take a look into this issue and reassign if this issue is not related to your change.

Thanks..!!

 Issue 756096  has been merged into this issue.

Nifty.

So, what's happening here:
Extension bindings are (currently) constructed in JavaScript with lazy getters (i.e., a stub property is added which is replaced with a real value on an as-needed basis; and that real value is created in JS).  While debugging, JS is "suspended".  So when you pause JS in a debugger and then try to examine a new property for the first time, it will try to perform the lazy getter, which will then try to execute JS, and subsequently fail (because JS is suspended).

Why this worked before is because extension bindings didn't execute JS suspension.  This led to other bugs.

Looking farther ahead, the right answer to this is that extension bindings will be entirely native, and this will just work naturally, because it won't execute any script.  Unfortunately, while the core of the bindings system is pretty much ready, converting each of the <n> custom bindings we have from JS to C++ will take time.

So, what can we do here?  It seems like these are a bit fundamentally at odds - we need to respect script suspension, but also execute script to return a value while suspended.  I'm not sure if there would be a way of detecting whether the getter was activated for devtools, and, if so, allowing nested script execution in that case (or if that would be something we'd want to do).

Plus some folks for thoughts.

I think it would make more sense to allow running extension bindings because it is not an author script. (For example, scripts typed in DevTools are running while the document is suspended but it makes sense. Extension bindings are sort of the scripts.)

> Why this worked before is because extension bindings didn't execute JS suspension.  This led to other bugs.

What were the bugs?

> I think it would make more sense to allow running extension bindings because it is not an author script. (For example, scripts typed in DevTools are running while the document is suspended but it makes sense. Extension bindings are sort of the scripts.)

hmm... this makes me a bit nervous because extension bindings are often, in some way, potentially visible to author script (we've had a ton of bugs around this where author script was executed when it shouldn't be, such as issue 628942).  This seems like it opens the door to those.  We could say that only the bindings-instantiation code is allowed to run during these times, but we'd still be at risk of having author script tampering with it.  Maybe that's an acceptable risk, since we should be striving to lock down that code anyway?

> What were the bugs?
 issue 629431  is one of the main ones that tracks the effort to respect script suspension, but there were others that were similar.

Thanks for the clarification!

Would it be a terrible idea to leave this bug as is until we convert all JS extension bindings to C++?

If it's not acceptable, would it be an option to allow running the extension bindings only when the document is suspended by DevTool's breakpoint? (i.e., forbid running the extension bindings when the document is suspended in other ways; e.g., page load deferrer)

> Would it be a terrible idea to leave this bug as is until we convert all JS extension bindings to C++?

I'm not entirely opposed to it - this has been broken since M55.  If there's a reasonable fix, that would obviously be preferable, but I don't think leaving things in this state for a bit longer is inviable.

> would it be an option to allow running the extension bindings only when the document is suspended by DevTool's breakpoint? (i.e., forbid running the extension bindings when the document is suspended in other ways; e.g., page load deferrer)

I'd be supportive of that, but I don't know if we have that info from the extensions layer.  Do you know how we determine that?

>> would it be an option to allow running the extension bindings only when the document is suspended by DevTool's breakpoint? (i.e., forbid running the extension bindings when the document is suspended in other ways; e.g., page load deferrer)
>
> I'd be supportive of that, but I don't know if we have that info from the extensions layer.  Do you know how we determine that?

I think we need to introduce Page::SetPausedByDevTools (in addition to the existing Page::SetPaused) to distinguish the two cases (i.e., paused by DevTools or paused by other things). Then expose that info to SuspendableScriptExecutor.

This is technically doable but adds some complexity to the architecture.

I'd be supportive of that approach, and happy to tackle the extensions side, but I'd probably want someone more familiar with the blink half to handle that portion.  I don't have a good estimation of how much work it would be for if you consider it worthwhile (the extensions side is probably doable in a single moderate size CL).  Otherwise, we can add this to the list of things that will be fixed with native bindings.

Yeah, I'd prefer fixing this by implementing native bindings unless there's an urgent need to fix this :)

As a temporary solution in case you choose to wait for C++ rewrite, can devtools simply inform that bindings/value/whatever is unavailable in paused state and will be initialized on continuing? That's for the hover tooltip and other places the user may access in paused state like maybe watches, console. 

>> it will try to perform the lazy getter, which will then try to execute JS, and subsequently fail (because JS is suspended)

Could you elaborate on this bit / point to the source? I would think that suspending should take place on the higher level / call site. If we are reaching lazy getter, js is already being executed, so there is no point in suspending anything...

Regarding the tab/extension crash in the merged  issue 756096 , will it be solved by the work done here?
I've bisected that one to r445814 "DevTools: introduce object previews experiment" 
(the corresponding info added in that report).

@15: It's rather roundabout, but the basic flow is as follows:

The lazy getter is set on the chrome object through ModuleSystem::SetLazyField:
https://chromium.googlesource.com/chromium/src/+/955c32907a2c9921065fa628e0b8b5b28a7afe6c/extensions/renderer/module_system.cc#501

DevTools then tries to evaluate the lazy field, which leads eventually to ModuleSystem::LazyFieldGetterInner and then to ModuleSystem::Require.  ModuleSystem::Require calls ModuleSystem::RequireForJSInner, which calls ModuleSystem::LoadModule:

https://chromium.googlesource.com/chromium/src/+/955c32907a2c9921065fa628e0b8b5b28a7afe6c/extensions/renderer/module_system.cc#706

ModuleSystem::LoadModule wraps a JS resource in a function, and then calls that through the ScriptContext, which uses blink::WebFrame::RequestExecuteV8Function:
https://chromium.googlesource.com/chromium/src/+/955c32907a2c9921065fa628e0b8b5b28a7afe6c/extensions/renderer/script_context.cc#205

This respects suspended JS, and thus won't execute while dev tools is open.