WebExtension eval's fail to load in the debugger

RESOLVED FIXED in Firefox 57

Status

enhancement
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jlast, Assigned: jlast)

Tracking

(Depends on 1 bug)

unspecified
Firefox 57
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

We see a couple of issues with web extension evals. 

1. the URL is difficult to interpret
2. the source does not have an inferred contentType, which makes it impossible to load the source, even though the actor has the text.

GH issue: https://github.com/devtools-html/debugger.html/issues/3899
GIF: http://g.recordit.co/qOadNPSCV1.gif

In the case of React Devtools, the content script looks for React w/ an eval every second. This causes the debugger to receive a new source every second, where it can't load the source text. Interestingly, every eval'd source has the same actor ID. I don't know if this was intentional or not.

What does this patch do:

* It sets an extension URL that is easier to make sense of on the client. 
* It updates the server contentType inference to assume that the eval is contentType javascript

Here is a happy screenshot of the source text loaded:

https://shipusercontent.com/4131a2bcb3264ed8eb5a48c42bf1cbfe/Screen%20Shot%202017-09-08%20at%2012.13.45%20PM.png
Posted patch extension-text.patch (obsolete) — Splinter Review
Attachment #8906012 - Flags: review?(lgreco)
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Priority: -- → P2
Posted patch extension-text-2.patch (obsolete) — Splinter Review
Attachment #8906012 - Attachment is obsolete: true
Attachment #8906012 - Flags: review?(lgreco)
Attachment #8906075 - Flags: review?(lgreco)
Posted patch extension-text-4.patch (obsolete) — Splinter Review
fixing an eslint issue
Attachment #8906075 - Attachment is obsolete: true
Attachment #8906075 - Flags: review?(lgreco)
Attachment #8906100 - Flags: review?(lgreco)
Comment on attachment 8906100 [details] [diff] [review]
extension-text-4.patch

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

Thanks for taking care of this, this fix is going to be very helpful to make it easier to debug the behavior of the devtools addons.

The changes applied to the WebExtensionInspectedWindowActor looks good to me, it would be nice if a DevTools peer could take another look at the change applied
to TabSources (the change looks more than reasonable to me, I'm only wondering if once we have been populating the introductionType correctly, then we should to also handle it explicitly elsewhere).

::: devtools/server/actors/utils/TabSources.js
@@ +301,5 @@
>      } else if (source.introductionType === "wasm") {
>        // Wasm sources are not JavaScript. Give them their own content-type.
>        spec.contentType = "text/wasm";
> +    } else if (source.introductionType === "debugger eval") {
> +      spec.contentType = "text/javascript";

This seems absolutely reasonable to be, but it would be better to be also reviewed by a DevTools peer (e.g. maybe ochameau could take a brief look at it, just to double-check it).
Attachment #8906100 - Flags: review?(lgreco) → review+
Comment on attachment 8906100 [details] [diff] [review]
extension-text-4.patch

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

Can you split this in 2 changesets? The TabSources change is not related to the other one. r=me on the other one.

::: devtools/server/actors/utils/TabSources.js
@@ +300,5 @@
>        spec.isInlineSource = true;
>      } else if (source.introductionType === "wasm") {
>        // Wasm sources are not JavaScript. Give them their own content-type.
>        spec.contentType = "text/wasm";
> +    } else if (source.introductionType === "debugger eval") {

Right below this block we do something similar. if (url) { ... } we set spec.contentType for a few use cases, including url === "debugger eval code".

Shouldn't this be mutualized? Is url === "debugger eval code" not redundant with source.introductionType === "debugger eval". From a quick test it seems to be the case. So maybe simply replace `url === "debugger eval code"` by source.introductionType === "debugger eval"

If they need to be handled differently, then add a comment explaining why.
Attachment #8906100 - Flags: review+
Posted patch extension-text-5.patch (obsolete) — Splinter Review
Attachment #8906100 - Attachment is obsolete: true
Attachment #8906740 - Flags: review?(jdescottes)
I created a new bug for doing the URL update: https://bugzilla.mozilla.org/show_bug.cgi?id=1398911

It'll be nice to do it in a different pass when we're not worried about 56/57...
Depends on: 1399064
Comment on attachment 8906740 [details] [diff] [review]
extension-text-5.patch

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

::: devtools/server/actors/utils/TabSources.js
@@ +301,5 @@
>      } else if (source.introductionType === "wasm") {
>        // Wasm sources are not JavaScript. Give them their own content-type.
>        spec.contentType = "text/wasm";
> +    } else if (source.introductionType === "debugger eval") {
> +      spec.contentType = "text/javascript";

I created Bug 1399064 to cleanup the duplication between this new code and the if (url === "debugger eval code"). Please add a comment along the lines of 

> // All debugger eval code should have a text/javascript content-type. See Bug 1399064
Attachment #8906740 - Flags: review?(jdescottes) → review+
Added comment.
Attachment #8906740 - Attachment is obsolete: true
Attachment #8908283 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/952bd76f99bc
WebExtension eval's fail to load in the debugger. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/952bd76f99bc
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.