Full Text Bug Listing

Joseph Pecoraro

Reported 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

Radar WebKit Bug Importer

Comment 1 2015-02-09 15:24:13 PST

<rdar://problem/19773092>

Joseph Pecoraro

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

Timothy Hatcher

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

Brian Burg

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

Brian Burg

Comment 5 2015-02-11 11:33:50 PST

Created attachment 246400 [details] Proposed Fix

Brian Burg

Comment 6 2015-02-11 11:34:50 PST

Test case: http://www.apple.com/imac-with-retina/5k.html

WebKit Commit Bot

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

Joseph Pecoraro

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

Joseph Pecoraro

Comment 9 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)?

Brian Burg

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

Brian Burg

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

Brian Burg

Comment 12 2015-02-12 07:56:18 PST

Created attachment 246442 [details] Proposed Fix (style nits)

WebKit Commit Bot

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

Brian Burg

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

Brian Burg

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

Timothy Hatcher

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

Brian Burg

Comment 17 2015-02-12 10:42:27 PST

Committed r179999: <http://trac.webkit.org/changeset/179999>

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Bug 141405