Bug 192731 – Web Inspector: m3u8 content not shown, it should be text

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

m3u8 content not shown, it should be text

Steps to Reproduce:
1. Inspect https://www.mtv.ca/shows/jersey-shore-family-vacation/episode/1552441/ronnie-magros-series-of-unfortunate-events/
2. Show Network Tab
3. Wait until after commercials for m3u8 resource to show up
4. Select m3u8
  => Expected to see text content, saw error rpage

Notes:
• mime type is "application/x-mpegurl"

[Radar WebKit Bug Importer]

<rdar://problem/46747728>

[Joseph Pecoraro]

Also on:
https://developer.jwplayer.com/tools/stream-tester/

MimeType:
application/vnd.apple.mpegurl

---

I'm seeing NetworkResourcesData::ResourceData::removeContent get called on the ResourceData that was decoded earlier. And even allowing the content to stay around the frontend seems like it might have an issue with it.

[Joseph Pecoraro]

The frontend has this list:

    // MPEG playlist
    "application/vnd.apple.mpegurl": "m3u8",
    "application/mpegurl": "m3u8",
    "application/x-mpegurl": "m3u8",
    "audio/mpegurl": "m3u",
    "audio/x-mpegurl": "m3u",

Both the backend and frontend should know to treat this as text.

[Joseph Pecoraro]

Created attachment 357383 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 357384 [details]
[IMAGE] m3u8 example

[EWS Watchlist]

Comment on attachment 357383 [details]
[PATCH] Proposed Fix

Attachment 357383 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10408515

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall.https.html

[EWS Watchlist]

Created attachment 357392 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Devin Rousso]

Comment on attachment 357383 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=357383&action=review

r=me, nice catch :)

> Source/WebCore/inspector/NetworkResourcesData.cpp:190
> +    if (content.isEmpty())

Should this be `isNull`?  Early returning with an empty string could mean that we return the error "No data found for resource with given identifier", as we wouldn't return true for `NetworkResourceData::ResourceData::hasContent`.  Isn't an empty string "" considered a valid response (e.g. a request that just returns 200)?

> Source/WebCore/platform/MIMETypeRegistry.cpp:506
> +        auto subtype = StringView { mimeType }.substring(applicationLength);

Is there a reason to use this syntax over `StringView(mimeType)`?

> Source/WebCore/platform/MIMETypeRegistry.cpp:514
> +        auto subtype = StringView { mimeType }.substring(audioLength);

Ditto (>506).

> Source/WebInspectorUI/UserInterface/Base/MIMETypeUtilities.js:325
>      if (mimeType.startsWith("application/"))
>          return mimeType.endsWith("script") || mimeType.endsWith("json");

We should probably modify this to use `extension` and just check for "js" and "json" (also maybe "ps").

[Blaze Burg]

Comment on attachment 357383 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=357383&action=review

How can we write a test for this?

>> Source/WebCore/platform/MIMETypeRegistry.cpp:506
>> +        auto subtype = StringView { mimeType }.substring(applicationLength);
> 
> Is there a reason to use this syntax over `StringView(mimeType)`?

o_O

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #8)
> Comment on attachment 357383 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357383&action=review
> 
> r=me, nice catch :)
> 
> > Source/WebCore/inspector/NetworkResourcesData.cpp:190
> > +    if (content.isEmpty())
> 
> Should this be `isNull`?  Early returning with an empty string could mean
> that we return the error "No data found for resource with given identifier",
> as we wouldn't return true for
> `NetworkResourceData::ResourceData::hasContent`.  Isn't an empty string ""
> considered a valid response (e.g. a request that just returns 200)?

Hmm, good idea. I'll try this.

> > Source/WebCore/platform/MIMETypeRegistry.cpp:506
> > +        auto subtype = StringView { mimeType }.substring(applicationLength);
> 
> Is there a reason to use this syntax over `StringView(mimeType)`?

Hmm, I copied/pasted this from the function above this.

> > Source/WebInspectorUI/UserInterface/Base/MIMETypeUtilities.js:325
> >      if (mimeType.startsWith("application/"))
> >          return mimeType.endsWith("script") || mimeType.endsWith("json");
> 
> We should probably modify this to use `extension` and just check for "js"
> and "json" (also maybe "ps").

I'll see if this will work.

[Joseph Pecoraro]

(In reply to Brian Burg from comment #9)
> Comment on attachment 357383 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357383&action=review
> 
> How can we write a test for this?

I considered writing a test for `shouldTreatMIMETypeAsText` but maybe just XHR loading an m3u8 and fetching content will be good enough.

[Joseph Pecoraro]

> > We should probably modify this to use `extension` and just check for "js"
> > and "json" (also maybe "ps").
> 
> I'll see if this will work.

I like the current detection of "application/*script" because if someone sent variants on these it would still work without knowing how to translate to an extension:

        "text/x-coffeescript": "coffee",
        "text/livescript": "ls",
        "text/typescript": "ts",
        "application/postscript": "ps",

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #12)
> I like the current detection of "application/*script" because if someone sent variants on these it would still work without knowing how to translate to an extension:
I missed that it was using `endsWith` and not just `===`.  That is fine.  I suggested using "js" and "json" because of weird MIME types like "text/javascript1.5", which would be covered if we added `extension === "js"`.  Doing both sounds like a good cover of all the bases.

[Joseph Pecoraro]

Created attachment 357518 [details]
[PATCH] Proposed Fix

Added tests for `WI.shouldTreatMIMETypeAsText`. Briefly tried to add a test loading a m3u8 but loading as XHR worked even without the patch so I abandoned this approach to testing. Ideally we should have a test though =(.

[WebKit Commit Bot]

Comment on attachment 357518 [details]
[PATCH] Proposed Fix

Rejecting attachment 357518 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 357518, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/10450631

[Joseph Pecoraro]

https://trac.webkit.org/r239343