Debugger statement is ignored in iframes removed from the document

[Caio Lima]

Hi DevTools people,

I sent a patch https://chromium-review.googlesource.com/c/chromium/src/+/2965275 couple of weeks ago to address the bug reported at https://bugs.chromium.org/p/chromium/issues/detail?id=1015462. I already got review from some of you and there were some errors that were found by CI that required some investigation. I spent the last weeks investigating the failures to understand if we should just rebase them or if it requires more changes to fix the issue.

The change is pretty simple, where we are not posting the message `Runtime.executionContextWasDestroyed`  when we are disposing the context from `LocalWindowProxy`. As we can see on https://bugs.chromium.org/p/chromium/issues/detail?id=1015462, the reason to not consider that the context of detached iframe was destryod is to allow debugger inspection on code running on such frame, in order to improve the developer experience when working with the Membrane pattern.

This change has impacts on how CDP and DevTools front-end tests are written now. Most of the failures reported there are because we are delaying `Runtime.executionContextWasDestroyed` message to be sent when the context is really collected by GC, instead of when a frame gets detached, causing difference on expected results for `uiSourceCodes` and from Console’s context selector. It’s possible to see on DevTools frontend that even after the frame detachment, we still can select its context, being able to evaluate and inspect code there. I suspect that this is also visible to other CDP clients.

I think we have 2 (maybe more) options to follow:

1. The first and simplest option is that we edit binding tests and integration tests expectations to take in consideration that the context will still be around after frame detachment, doing nothing on DevTools side (i.e as the patch is right now).

2. The other option is not quite clear how it should be done, but we could change DevTools to understand that we have a context alive even if its frame is detached, allowing users to inspect code there, but making clear on UI that this is a context from a detached frame.

Both solutions would change on how `Runtime.executionContextWasDestroyed` is sent to DevTools clients, but the latter makes it clear to users that such context is in an “exceptional” state. Besides Context selector option on Devtools frontend, there’s not much that is visible to users besides the ability to use debugger to inspect code code evaled from a detached iframe. I can't assure how visible this is for CDP users besides tests failures there.

I think it’s important to reiterate with you that being able to run `eval` from a detached iframe should be allowed per ECMA262 spec and there are important use cases relying on it right now. ToT Chromium allows the execution of eval, however it’s not possible to debug such code. Other browsers allow to either run eval and inspect code from detached frames, but I noticed they don't have something similiar to Chromium's context selector, so there's not much I could compare regarding UI differences between them.

I’d like to get some thoughts on this before moving forward with this patch. It would be nice if we could arrange an one-time meeting so we could discuss about it. What do you think?

Caio.

[Sigurd Schneider]

+Andrey Kosyakov 

--
You received this message because you are subscribed to the Google Groups "devtools-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to devtools-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/devtools-dev/032cfdf3-4a7e-44f2-bee4-99442d680cd3n%40chromium.org.

--

Sigurd Schneider | Software Engineer | Chrome - V8 | sig...@google.com

 Germany GmbH / Erika-Mann-Straße 33 / 80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde.

    

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.

[Caio Lima]

Moving this thread up.

--
You received this message because you are subscribed to a topic in the Google Groups "devtools-dev" group.
To unsubscribe from this topic, visit https://groups.google.com/a/chromium.org/d/topic/devtools-dev/Cc20iPmhcDc/unsubscribe.
To unsubscribe from this group and all its topics, send an email to devtools-dev...@chromium.org.

[Yang Guo]

+Benedikt Meurer do you have an opinion on this?

You received this message because you are subscribed to the Google Groups "devtools-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to devtools-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/devtools-dev/D26DEF2E-DFD5-473C-9615-91A2CC0E64E5%40igalia.com.

[Caio Lima]

I also updated the tests that were failing before, forcing a GC after frame detachment. The reason to do it was to assert that those contexts will be collected by GC if there’s nothing holding their reference. A couple of other tests were also failing because we still want to sent `Runtime.executionContextDestroyed` when we are executing `ClerForNavigate` and `ClearForSwap`. 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/devtools-dev/D26DEF2E-DFD5-473C-9615-91A2CC0E64E5%40igalia.com.

[Benedikt Meurer]

Personally, I think option 1 sounds like the best path forward. If we learn that we need more smarts, we can always add them later (unless I'm missing something).

Regarding the review (and ideally generally next steps), let's wait for +Simon Zünd to have a look next week.

cheers,

Benedikt

--

Benedikt Meurer

Chromium DevTools TL

bme...@chromium.org

Google Germany GmbH

Erika-Mann-Straße 33

[Andrey Kosyakov]

This looks like a reasonable change to me -- although, judging by the test failures, it is somewhat protocol breaking, the semantics that these tests used to rely on are not something we intended to guarantee, and the behavior that the CL introduces looks like the right thing to do, so we should only be concerned if it  happens to cause a large-scale breakage -- but I wouldn't expect a lot of clients to be relying on a corner case like this. I've run the puppeteer tests just in case, as well as tests for some of our internal systems that rely heavily on CDP, and these seem to be fine, so lgtm from my perspective. I haven't looked into the front-end side of things, but considering we already deal with plenty of cases where an execution context is not a frame, I would expect we would be managing the console drop-down solely based on execution context lifecycle events and use frame events just for augmenting the details of the context, in which case "option 2" would be not too difficult to implement.

Best regards,

Andrey.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/devtools-dev/CAGowDnZLO2h%3D%2BxOo4-zWx0rpEg62U9ySW41ALZEPdX1LpmN3qg%40mail.gmail.com.

[Simon Zünd]

My concerns stated on the CL were addressed. I agree with Benedikt and Andrey that we can go with Option 1 and land the CL after branch cut on Thursday.

After that we should keep an eye on the layout tests, given that they rely on GC now and might become flaky. Once the change has made it into canary we can also ping salesforce folks on the corresponding crbug to see if the change works for them. I assume that they would be the people running first into edge cases that are not covered/working yet should there be any.

Cheers, Simon

[Caio Lima]

Hi all.

I updated the CL with reviews I’ve got so far. I’m wondering if there’s something missing on my side to move forward with this change. It seems that I need review from a compile of owners, but when I use “SUGGEST OWNERS” feature, it lists people that were not involved on our discussions. Could somebody help me understand what’s missing?

- Caio.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/devtools-dev/CACswSC3qZ58uF7NbqteWCxncXiUFivp9x14KSm10-wy3C%2BCG7Q%40mail.gmail.com.

[Caio Lima]

In addition to that. Would it be possible to get an invite to join Chromium Slack channel?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/devtools-dev/7796CEBB-8491-4782-AD7B-B7C3DC1A9EDA%40igalia.com.

[Andrey Kosyakov]

On Tue, Jul 27, 2021 at 12:02 PM Caio Lima <cl...@igalia.com> wrote:

In addition to that. Would it be possible to get an invite to join Chromium Slack channel?

On Jul 27, 2021, at 15:13, Caio Lima <cl...@igalia.com> wrote:

Hi all.

I updated the CL with reviews I’ve got so far. I’m wondering if there’s something missing on my side to move forward with this change. It seems that I need review from a compile of owners, but when I use “SUGGEST OWNERS” feature, it lists people that were not involved on our discussions. Could somebody help me understand what’s missing?

Ah, apologies for the confusion, I was waiting for some of the bindings/ OWNERS to review your CL, but it looks like you don't have any owners there on the reviewers list. I stamped your CL and added Nate for bindings/.

As for joining chromium slack, you should be able to do it using your @igalia.com email, as per https://www.chromium.org/developers/slack (Folks on allow-listed email domains from AUTHORS can directly use https://chromium.slack.com/signup, e.g. @microsoft.com or @mozilla.com)

Best regards,

Andrey.