1398210 - WebExtension eval's fail to load in the debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P2)

Description

[Jason Laster [:jlast]]

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

Comment 1

[Jason Laster [:jlast]]

Attachment: extension-text.patch

Comment 2

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf443fc94299b7b7ef899f89472175271aaefa71

Comment 3

[Jason Laster [:jlast]]

Attachment: extension-text-2.patch

Comment 4

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05fcdc164485802b62985d7eb148b63481e95def

Comment 5

[Jason Laster [:jlast]]

Attachment: extension-text-4.patch

fixing an eslint issue

Comment 6

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=132556bcf82920fb4c8e1e6cac4de383477963f2

Comment 7

[Luca Greco [:rpl] [:luca] [: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).

Comment 8

[Julian Descottes [:jdescottes]]

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.

Comment 9

[Jason Laster [:jlast]]

Attachment: extension-text-5.patch

Comment 10

[Jason Laster [:jlast]]

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...

Comment 11

[Julian Descottes [:jdescottes]]

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

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: bug1398210.amended.patch

Added comment.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/952bd76f99bc
WebExtension eval's fail to load in the debugger. r=jdescottes

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/952bd76f99bc