Bug 144107 – Web Inspector: Inspector hangs when trying to view a large XHR resource

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

[Brian Burg]

Steps to reproduce:

1. Go to https://build.webkit.org/dashboard/metrics.html
2. Request a month's worth of data.
3. Open the inspector.
4. Select one of the XHR resources labelled '_all' (Should be >6MB in size)
5. :hang:

I'm not sure what's going on here, Codemirror should be able to display a resource that large without much trouble. (Right?)

[Radar WebKit Bug Importer]

<rdar://problem/20669463>

[Timothy Hatcher]

<rdar://problem/19882575>

[Joseph Pecoraro]

I created a test page that loads a 9MB JSON Resource (an Object with 300000 properties. 

Before my changes selecting the resource in Web Inspector's Resource sidebar we took about 10 seconds to show the resource with the UI being completely frozen for about 9s.

After, we load the resource in under 0.5s, with the UI being hung for maybe 100ms. Most of the time is loading the content from the backend. Formatting is done async in a worker. We avoid asking CodeMirror to layout multiple times. We avoid trying to create CodeMirror markers for colors/gradients etc.

I will use this bug for the final part of the work. Hooking up the async formatter in TextEditor/SourceCodeTextEditor.

[Joseph Pecoraro]

Created attachment 275869 [details]
[PATCH] Proposed Fix

Here we go!

There is one known issue right now. When reloading with a breakpoint selected, the content does not reveal the breakpoint line in the center of the screen like it is supposed to. I want to investigate that before landing.

There is one possible issue. The BreakpointTreeElement being in the UI even though it's Breakpoint.sourceCodeLocation.sourceCode being null is new to this patch. I haven't investigated the root cause of this regression, but I painted over it with the changes to ContentView / NavigationBar restoration. It would be nice to understand what changed here but I'm not as concerned about this compared to the previous issue.

[Timothy Hatcher]

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

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

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:141
> +        this._format(formatted).then(callback).catch(handlePromiseException);

Why make this be a callback and not return the promise so callers can chain?

> Source/WebInspectorUI/UserInterface/Views/TextResourceContentView.js:206
> +        this._textEditor.setFormatted(activated);

Maybe keep the setter for the non-callback cases?

[Blaze Burg]

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

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

Great work!

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:157
> +        if (!resolvedRepresentedObject)

I would add a comment here to summarize when we could expect this to fail (and not be a bug).

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:207
> +        let contentView = this.contentBrowser.showContentViewForRepresentedObject(treeElement.representedObject);

Or here.

>> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:141
>> +        this._format(formatted).then(callback).catch(handlePromiseException);
> 
> Why make this be a callback and not return the promise so callers can chain?

I was going to comment the same. And, maybe call it updateFormattedState so it doesn't look like a setter. It seems weird that a setter has a callback or promise.

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:740
> +                this._undoFormatting(beforePrettyPrintState, resolve);

Aw man, I was expecting it to be promises all the way down! I guess you'd have to change WorkerFormatter, so this is fine.

[Joseph Pecoraro]

(In reply to comment #5)
> Comment on attachment 275869 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275869&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/TextEditor.js:141
> > +        this._format(formatted).then(callback).catch(handlePromiseException);
> 
> Why make this be a callback and not return the promise so callers can chain?

I made this return the promise. By using a single then here we could catch any errors with this one catch handler. Otherwise we'd have to add a catch handler everywhere we add a then, which is kinda unfortunate, since there is no `window.onerror` for Promise exceptions.

I've considered making something like this to ensure we always have a catch handler. Its a dirty kind of Promise.oncatch, but the problem is what if we reject a promise intentionally, then we get the uncaught exception dialog incorrectly...

    (function() {
        let originalThen = Promise.prototype.then;
        Promise.prototype.then = function(resolveHandler, rejectHandler) {
            if (!rejectHandler)
                return originalThen.call(this, resolveHandler, handlePromiseException);
            return originalThen.call(this, resolveHandler, rejectHandler);
        }
    })();



(In reply to comment #6)
> Comment on attachment 275869 [details]
> [PATCH] Proposed Fix
> 
> >> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:141
> >> +        this._format(formatted).then(callback).catch(handlePromiseException);
> > 
> > Why make this be a callback and not return the promise so callers can chain?
> 
> I was going to comment the same. And, maybe call it updateFormattedState so
> it doesn't look like a setter. It seems weird that a setter has a callback
> or promise.

Yeah, the name here was pretty bad. I used your suggestion.

> > Source/WebInspectorUI/UserInterface/Views/TextEditor.js:740
> > +                this._undoFormatting(beforePrettyPrintState, resolve);
> 
> Aw man, I was expecting it to be promises all the way down! I guess you'd
> have to change WorkerFormatter, so this is fine.

It could be, I just didn't trust myself to do it. And given it is all internal code at this point, it seemed fine to not do it.

I wonder what the effect will be of adding all these micro tasks where there weren't any before. Until I better understand microtask checkpoints and Promise chaining, I'm going to stick to what I know works. This patch was already taking me a long time.

[Joseph Pecoraro]

http://trac.webkit.org/changeset/199169