Bug 141405 - Web Inspector: Large background image fails to load in inspector
Summary: Web Inspector: Large background image fails to load in inspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-02-09 15:23 PST by Joseph Pecoraro
Modified: 2015-02-12 10:42 PST (History)
9 users (show)

See Also:


Attachments
Proposed Fix (13.61 KB, patch)
2015-02-11 11:33 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (style nits) (13.59 KB, patch)
2015-02-12 07:56 PST, Brian Burg
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-02-09 15:23:25 PST
* SUMMARY
Large background image fails to load in inspector.

* TEST
<style>body { background: url(x.png) }</style>
<body>Test</body>

* STEPS TO REPRODUCE
1. Create a large image (e.g. >2 MB) named "x.png" aside test page.
2. Inspect test page
  => "x.png" doesn't show up in inspector
Comment 1 Radar WebKit Bug Importer 2015-02-09 15:24:13 PST
<rdar://problem/19773092>
Comment 2 Joseph Pecoraro 2015-02-09 15:29:58 PST
Looks like we only support up to 1 MiB data URLs:

    get contentURL()
    {
        const maximumDataURLSize = 1024 * 1024; // 1 MiB

        // If content is not available or won't fit a data URL, fallback to using original URL.
        var content = this.content;
        if (content === null || content.length > maximumDataURLSize)
            return this._url;

        return "data:" + this.mimeTypeComponents.type + (this.contentIsBase64Encoded ? ";base64," + content : "," + encodeURIComponent(content));
    },

Can we bump that, or do we need to come up with another solution here?

In this particular case, if the page is a file:/// the inspector cannot open that image. Likewise, if inspecting a remote target, the URL may not be loadable by the inspector.
Comment 3 Timothy Hatcher 2015-02-10 12:52:14 PST
We can bump it. I picked it at random. I wonder if there is a limit on URL size?

Using Blob and URL.createObjectURL() would be the best solution. We would just need to call URL.revokeObjectURL() at the right time (ContentView.close?). We didn't have support for Blob and createObjectURL when we did this 1 MiB data URL thing.
Comment 4 Brian Burg 2015-02-10 14:11:40 PST
(In reply to comment #3)
> We can bump it. I picked it at random. I wonder if there is a limit on URL
> size?
> 
> Using Blob and URL.createObjectURL() would be the best solution. We would
> just need to call URL.revokeObjectURL() at the right time
> (ContentView.close?). We didn't have support for Blob and createObjectURL
> when we did this 1 MiB data URL thing.

I was just looking at this code as well, since my element tracking feature sends many screenshots which can be >1MB. Simply bumping the cap will not help that much since photograph-level details can create pngs with sizes over 5 MiB. Plus there is the URL length issue, which seems like skating on thin ice and can make 2nd level inspector unusable if you happen to log the data. It would be nice to keep protocol messages to some fixed size

So, the new way would be to send over in chunks, turn that into a blob, then (optionally) turn that into an ObjectURL? It would be nice to make the protocol API agnostic to the contents of the blob. For example, some uses may already prepend the mime type, while others create it on the inspector side.

I propose altering the protocol to support multi-part messages for blob-like objects. Suppose a command exists like this:

{
    "name": "getResourceContent",
    "params": [
        {"name": "id", "type": "integer"},
        {"name": "url", "type": "string"}
    ],
    "returns" [
        {"name": "data", "type": "blob"},
        {"name": "dummyNumber", "type": "number"}
    ]
}

The protocol traffic would look like this:

<------ {"id": 33, "method": "DOM.getResourceContent", "params":{id: 42, url: "https://apple.com"}}
...
...
------> {"id": 33, "result": {"data": {"blobId": 99}, "dummyNumber": 123}, "multipart": true}
------> {"id": 33, "part": {"blobId": 99, "data": "60oeubeu0e6ueene6..."}, "multipart": true}
------> {"id": 33, "part": {"blobId": 99, "data": "92409646eeaffbbce..."}, "multipart": true}
------> {"id": 33, "part": {"blobId": 99, "data": "13941021406aef064..."}}

Multipart messages continue with the same sequence id. The "multipart":true key indicates that more chunks will be sent. When a chunk arrives without that key, a Blob object is created with the chunks, inserted into the result object, and dispatched in the frontend from InspectorBackend.

Note that this would cause the command reply to appear delayed relative to other replies whose commands were sent after getResourceContent. I don't think this is generally a problem, as the "async" command keyword can similarly delay messages and hasn't been a great source of bugs.
Comment 5 Brian Burg 2015-02-11 11:33:50 PST
Created attachment 246400 [details]
Proposed Fix
Comment 6 Brian Burg 2015-02-11 11:34:50 PST
Test case: http://www.apple.com/imac-with-retina/5k.html
Comment 7 WebKit Commit Bot 2015-02-11 11:36:52 PST
Attachment 246400 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Base/Utilities.js:1050:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Joseph Pecoraro 2015-02-11 18:41:07 PST
(In reply to comment #4)
> (In reply to comment #3)
> > We can bump it. I picked it at random. I wonder if there is a limit on URL
> > size?
> > 
> > Using Blob and URL.createObjectURL() would be the best solution. We would
> > just need to call URL.revokeObjectURL() at the right time
> > (ContentView.close?). We didn't have support for Blob and createObjectURL
> > when we did this 1 MiB data URL thing.
> 
> I was just looking at this code as well, since my element tracking feature
> sends many screenshots which can be >1MB. Simply bumping the cap will not
> help that much since photograph-level details can create pngs with sizes
> over 5 MiB. Plus there is the URL length issue, which seems like skating on
> thin ice and can make 2nd level inspector unusable if you happen to log the
> data. It would be nice to keep protocol messages to some fixed size
> 
> So, the new way would be to send over in chunks, turn that into a blob, then
> (optionally) turn that into an ObjectURL? It would be nice to make the
> protocol API agnostic to the contents of the blob. For example, some uses
> may already prepend the mime type, while others create it on the inspector
> side.
> 
> I propose altering the protocol to support multi-part messages for blob-like
> objects. Suppose a command exists like this:
> 
> {
>     "name": "getResourceContent",
>     "params": [
>         {"name": "id", "type": "integer"},
>         {"name": "url", "type": "string"}
>     ],
>     "returns" [
>         {"name": "data", "type": "blob"},
>         {"name": "dummyNumber", "type": "number"}
>     ]
> }
> 
> The protocol traffic would look like this:
> 
> <------ {"id": 33, "method": "DOM.getResourceContent", "params":{id: 42,
> url: "https://apple.com"}}
> ...
> ...
> ------> {"id": 33, "result": {"data": {"blobId": 99}, "dummyNumber": 123},
> "multipart": true}
> ------> {"id": 33, "part": {"blobId": 99, "data": "60oeubeu0e6ueene6..."},
> "multipart": true}
> ------> {"id": 33, "part": {"blobId": 99, "data": "92409646eeaffbbce..."},
> "multipart": true}
> ------> {"id": 33, "part": {"blobId": 99, "data": "13941021406aef064..."}}
> 
> Multipart messages continue with the same sequence id. The "multipart":true
> key indicates that more chunks will be sent. When a chunk arrives without
> that key, a Blob object is created with the chunks, inserted into the result
> object, and dispatched in the frontend from InspectorBackend.
> 
> Note that this would cause the command reply to appear delayed relative to
> other replies whose commands were sent after getResourceContent. I don't
> think this is generally a problem, as the "async" command keyword can
> similarly delay messages and hasn't been a great source of bugs.

I agree with Brian. This is useful, if there are data blobs that are so big, that ingestion of the large message causes us to miss "ticks". Splitting the large blob into chunks allows the frontend to be more responsive while handling a large otherwise synchronous message. Someone should enable the message timing statistics we have and load a page with large images and see if this really is a performance issue we are encountering, as it would be non-trivial.

That said, media resources might benefit from this as they will likely be large.
Comment 9 Joseph Pecoraro 2015-02-11 19:08:16 PST
Comment on attachment 246400 [details]
Proposed Fix

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

This seems pretty cool. Do we do a good job of showing ObjectURLs in the inspector? Do we even show them at all? This sounds like a great ER if a site is using a lot of blobs.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1048
> +function decodeBase64ToBlob(base64Data, contentType)

Nit: mimeType instead of contentType?

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1052
> +    var sliceSize = 1024;

Nit: const?

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1065
> +        for (var offset = begin, i = 0 ; offset < end; ++i, ++offset) {
> +            bytes[i] = byteCharacters[offset].charCodeAt(0);
> +        }

Style: No braces.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1069
> +}

I see this is similar to solutions on:
http://stackoverflow.com/questions/16245767/creating-a-blob-from-a-base64-string-in-javascript

Why does creating slices improve the performance of this? Does it have to do with memory layout?

> Source/WebInspectorUI/UserInterface/Models/Resource.js:262
> +        if (!this.content)
> +            return this._url;

Is this a suitable "ObjectURL"? Like, if URL.revokeObjectURL gets called on this (which could happen in this path), does it have any issues?

> Source/WebInspectorUI/UserInterface/Models/Resource.js:267
> +        console.assert(content instanceof Blob, content);
>  
> -        return "data:" + this.mimeTypeComponents.type + (this.contentIsBase64Encoded ? ";base64," + content : "," + encodeURIComponent(content));
> +        return URL.createObjectURL(content);

Hmm, dataURL might be nice for small things.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:660
> +            URL.revokeObjectURL(objectURL);

Since createObjectURL can return a URL string, does this always make sense?

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:55
> +        var objectURL = this.resource.createObjectURL();
>          this._imageElement = document.createElement("img");
> -        this._imageElement.src = this.resource.contentURL;
> +        this._imageElement.addEventListener("load", function() {
> +            URL.revokeObjectURL(objectURL);
> +        });
> +        this._imageElement.src = objectURL;
>  

What happens now when you have an image and drag / drop it out of the inspector. Does that still work as expected? Is it still named "Unknown.png" (something we really want to fix)?
Comment 10 Brian Burg 2015-02-12 07:43:52 PST
Comment on attachment 246400 [details]
Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1069
>> +}
> 
> I see this is similar to solutions on:
> http://stackoverflow.com/questions/16245767/creating-a-blob-from-a-base64-string-in-javascript
> 
> Why does creating slices improve the performance of this? Does it have to do with memory layout?

TBH, not exactly sure. There are many strategies for base64 to Blob, and this one seemed the simplest. I have no idea whether it's the fastest, but that can be improved later.

I would assume that preallocating an Array could be expensive because of its weird semantics. Other approaches reimplement atob() and charCodeAt() using ArrayBuffers, which also seems to be quite fast.

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:262
>> +            return this._url;
> 
> Is this a suitable "ObjectURL"? Like, if URL.revokeObjectURL gets called on this (which could happen in this path), does it have any issues?

It's not, but revokeObjectURL simply does nothing if the URL is not registered. See BlobRegistryImpl::unregisterBlobURL

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:660
>> +            URL.revokeObjectURL(objectURL);
> 
> Since createObjectURL can return a URL string, does this always make sense?

See above.

>> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:55
>>  
> 
> What happens now when you have an image and drag / drop it out of the inspector. Does that still work as expected? Is it still named "Unknown.png" (something we really want to fix)?

It still works. The filename is still Unknown.$ext, and in the Finder Get Info dialog, it says "Where from: blob:file:///ed4e613d-7c05-4424-b33d-fcc330a03db2, ". This seems like a separate issue.
Comment 11 Brian Burg 2015-02-12 07:55:30 PST
Comment on attachment 246400 [details]
Proposed Fix

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

>>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1069
>>> +}
>> 
>> I see this is similar to solutions on:
>> http://stackoverflow.com/questions/16245767/creating-a-blob-from-a-base64-string-in-javascript
>> 
>> Why does creating slices improve the performance of this? Does it have to do with memory layout?
> 
> TBH, not exactly sure. There are many strategies for base64 to Blob, and this one seemed the simplest. I have no idea whether it's the fastest, but that can be improved later.
> 
> I would assume that preallocating an Array could be expensive because of its weird semantics. Other approaches reimplement atob() and charCodeAt() using ArrayBuffers, which also seems to be quite fast.

At least in some JS engines, allocating too big of an Array will convert it to be a sparse map. Not sure if JSC does this.
Comment 12 Brian Burg 2015-02-12 07:56:18 PST
Created attachment 246442 [details]
Proposed Fix (style nits)
Comment 13 WebKit Commit Bot 2015-02-12 07:58:47 PST
Attachment 246442 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Base/Utilities.js:1050:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Brian Burg 2015-02-12 08:23:50 PST
(In reply to comment #8)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > We can bump it. I picked it at random. I wonder if there is a limit on URL
> > > size?
> > > 
> > > Using Blob and URL.createObjectURL() would be the best solution. We would
> > > just need to call URL.revokeObjectURL() at the right time
> > > (ContentView.close?). We didn't have support for Blob and createObjectURL
> > > when we did this 1 MiB data URL thing.
> > 
> > I was just looking at this code as well, since my element tracking feature
> > sends many screenshots which can be >1MB. Simply bumping the cap will not
> > help that much since photograph-level details can create pngs with sizes
> > over 5 MiB. Plus there is the URL length issue, which seems like skating on
> > thin ice and can make 2nd level inspector unusable if you happen to log the
> > data. It would be nice to keep protocol messages to some fixed size
> > 
> > So, the new way would be to send over in chunks, turn that into a blob, then
> > (optionally) turn that into an ObjectURL? It would be nice to make the
> > protocol API agnostic to the contents of the blob. For example, some uses
> > may already prepend the mime type, while others create it on the inspector
> > side.
> > 
> > I propose altering the protocol to support multi-part messages for blob-like
> > objects. Suppose a command exists like this:
> > 
> > {
> >     "name": "getResourceContent",
> >     "params": [
> >         {"name": "id", "type": "integer"},
> >         {"name": "url", "type": "string"}
> >     ],
> >     "returns" [
> >         {"name": "data", "type": "blob"},
> >         {"name": "dummyNumber", "type": "number"}
> >     ]
> > }
> > 
> > The protocol traffic would look like this:
> > 
> > <------ {"id": 33, "method": "DOM.getResourceContent", "params":{id: 42,
> > url: "https://apple.com"}}
> > ...
> > ...
> > ------> {"id": 33, "result": {"data": {"blobId": 99}, "dummyNumber": 123},
> > "multipart": true}
> > ------> {"id": 33, "part": {"blobId": 99, "data": "60oeubeu0e6ueene6..."},
> > "multipart": true}
> > ------> {"id": 33, "part": {"blobId": 99, "data": "92409646eeaffbbce..."},
> > "multipart": true}
> > ------> {"id": 33, "part": {"blobId": 99, "data": "13941021406aef064..."}}
> > 
> > Multipart messages continue with the same sequence id. The "multipart":true
> > key indicates that more chunks will be sent. When a chunk arrives without
> > that key, a Blob object is created with the chunks, inserted into the result
> > object, and dispatched in the frontend from InspectorBackend.
> > 
> > Note that this would cause the command reply to appear delayed relative to
> > other replies whose commands were sent after getResourceContent. I don't
> > think this is generally a problem, as the "async" command keyword can
> > similarly delay messages and hasn't been a great source of bugs.
> 
> I agree with Brian. This is useful, if there are data blobs that are so big,
> that ingestion of the large message causes us to miss "ticks". Splitting the
> large blob into chunks allows the frontend to be more responsive while
> handling a large otherwise synchronous message. Someone should enable the
> message timing statistics we have and load a page with large images and see
> if this really is a performance issue we are encountering, as it would be
> non-trivial.
> 
> That said, media resources might benefit from this as they will likely be
> large.

I turned on message timing and slow run loop warnings. I could not get any image to take more than 2ms to process, even the iMac 5K images (~5MB). So this may be necessary only once we start transferring very large resources like video and audio.
Comment 15 Brian Burg 2015-02-12 08:26:13 PST
(In reply to comment #9)
> Comment on attachment 246400 [details]
> Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246400&action=review
> 
> This seems pretty cool. Do we do a good job of showing ObjectURLs in the
> inspector? Do we even show them at all? This sounds like a great ER if a
> site is using a lot of blobs.

Right now the object URLS (typically like 'blob:${uuid}') are only shown in the Inspector if the URL is used to load a resource (font, image, whatever). Otherwise they don't show up. Blobs and Files themselves have no representation in the Inspector.
Comment 16 Timothy Hatcher 2015-02-12 10:06:05 PST
Comment on attachment 246400 [details]
Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1068
> +    }
> +    return new Blob(byteArrays, {type: contentType});

Nit: Newline

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:53
> +        this._imageElement.addEventListener("load", function() {
> +            URL.revokeObjectURL(objectURL);
> +        });

Nit: I'd make this one line.
Comment 17 Brian Burg 2015-02-12 10:42:27 PST
Committed r179999: <http://trac.webkit.org/changeset/179999>