Bug 192731 - Web Inspector: m3u8 content not shown, it should be text
Summary: Web Inspector: m3u8 content not shown, it should be text
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-14 17:41 PST by Joseph Pecoraro
Modified: 2018-12-18 11:19 PST (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (6.31 KB, patch)
2018-12-14 20:12 PST, Joseph Pecoraro
drousso: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
[IMAGE] m3u8 example (846.54 KB, image/png)
2018-12-14 20:12 PST, Joseph Pecoraro
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (11.97 MB, application/zip)
2018-12-14 22:05 PST, EWS Watchlist
no flags Details
[PATCH] Proposed Fix (10.84 KB, patch)
2018-12-17 19:29 PST, Joseph Pecoraro
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2018-12-14 17:41:57 PST
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"
Comment 1 Radar WebKit Bug Importer 2018-12-14 17:42:25 PST
<rdar://problem/46747728>
Comment 2 Joseph Pecoraro 2018-12-14 19:43:54 PST
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.
Comment 3 Joseph Pecoraro 2018-12-14 19:47:29 PST
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.
Comment 4 Joseph Pecoraro 2018-12-14 20:12:00 PST
Created attachment 357383 [details]
[PATCH] Proposed Fix
Comment 5 Joseph Pecoraro 2018-12-14 20:12:19 PST
Created attachment 357384 [details]
[IMAGE] m3u8 example
Comment 6 EWS Watchlist 2018-12-14 22:05:06 PST
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
Comment 7 EWS Watchlist 2018-12-14 22:05:08 PST
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
Comment 8 Devin Rousso 2018-12-17 10:33:55 PST
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").
Comment 9 Brian Burg 2018-12-17 10:38:23 PST
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
Comment 10 Joseph Pecoraro 2018-12-17 11:09:08 PST
(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.
Comment 11 Joseph Pecoraro 2018-12-17 11:09:59 PST
(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.
Comment 12 Joseph Pecoraro 2018-12-17 13:28:23 PST
> > 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",
Comment 13 Devin Rousso 2018-12-17 13:34:00 PST
(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.
Comment 14 Joseph Pecoraro 2018-12-17 19:29:00 PST
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 =(.
Comment 15 WebKit Commit Bot 2018-12-17 19:30:37 PST
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
Comment 16 Joseph Pecoraro 2018-12-18 11:19:04 PST
https://trac.webkit.org/r239343