Bug 135777

<rdar://problem/17965804>

Created attachment 241451 [details] [PATCH] Work in progress. Here is a basic attempt to make this change. There is a bug in this code. The attempt to create this._activeRequestContentPromise. For some reason when it comes time to resolve this in processContent, this._activeRequestContentPromise is undefined. I'm not sure why.

Comment on attachment 241451 [details] [PATCH] Work in progress. View in context: https://bugs.webkit.org/attachment.cgi?id=241451&action=review > Source/WebInspectorUI/UserInterface/Controllers/AnalyzerManager.js:104 > + sourceCode.requestContent().then(retrieveAnalyzerMessages.bind(this)); You could think about inlining some of these callback functions if they aren't used anywhere else. It seems like some of these async chains (especially further below) are manually hooked up now, and would require some more careful refactoring to fully exploit the promise-based task style. There has been some disagreement in the past as to whether it's better to explicitly bind |this| in said inlined functions, or capture something like |var manager = this;| and use it inside the function. > Source/WebInspectorUI/UserInterface/Models/SourceCode.js:129 > + if (!this._activeRequestContentPromise) I would add braces here for the multi-line true branch. > Source/WebInspectorUI/UserInterface/Models/SourceCode.js:131 > + if (this._contentReceived) In what other ways could this be already set? It seems that this gets more sticky as the number of states increases. Can we use promises in all places that request content from backend, rather than just here? > Source/WebInspectorUI/UserInterface/Models/SourceCode.js:138 > + this.requestContentFromBackendIfNeeded(); The promise will never resolve if reaching the else if or else branches. > Source/WebInspectorUI/UserInterface/Models/SourceCode.js:217 > + this.requestContentFromBackend().then(this._processContent); missing .bind(this) > Source/WebInspectorUI/UserInterface/Models/SourceCode.js:268 > + this._activeRequestContentPromise.resolve({ This won't work. |Promise.prototype.resolve| is not a method; more generally you can't modify the state of a promise from outside the function used to create the Promise. So, this implies that _processContent needs to go inside the function passed to |new Promise(...)|. That's why this refactoring is nontrivial.

Created attachment 241501 [details] [PATCH] Revised attempt. This actually seems to work. A second stored promise was utilized. I'm wondering though, I probably need to invalidate or remove these promises in case the content changes? What else would need to be considered?

Comment on attachment 241501 [details] [PATCH] Revised attempt. View in context: https://bugs.webkit.org/attachment.cgi?id=241501&action=review Getting better! As for invalidating the promise, I don't know how SourceCode lifetimes work. If the content keeps changing, then you can keep a map of revision# --> content promise and include the revision# in the parameters object? > Source/WebInspectorUI/UserInterface/Models/Resource.js:548 > { A convention I have started following (but which is still up for debate) is to annotate a function with what type of value the promise will resolve to. And, maybe add a more wordy comment if the type could be multiple things. > Source/WebInspectorUI/UserInterface/Models/Resource.js:552 > + return NetworkAgent.getResponseBody(this._requestIdentifier); Use .promise for now. > Source/WebInspectorUI/UserInterface/Models/Resource.js:559 > + return Promise.resolve(); Should this be Promise.reject()? I think that callers should be handling success and failure cases differently... and doing it with resolve/reject seems less sketchy than expecting 'null'. > Source/WebInspectorUI/UserInterface/Models/Script.js:159 > + makeSyntaxTreeAndCallCallback(parameters.error ? null : parameters.content); For example, this could be like so: this.requestContent() .then(function success(parameters) { ... }) .catch(function error(message) { ... }) Note that I put catch chained onto .then; if you give it as a second parameter (i.e., requestContent().then(fn succ() { ... }, fn err() { ... }) then you will miss any exceptions thrown by the then() callback. (also, I prefer putting .then on a newline with more indentation, but I think Joe and Tim prefer it as written. So, YMMV?) > Source/WebInspectorUI/UserInterface/Models/SourceCode.js:130 > + this._activeRequestContentPromise = new Promise(function (resolve, reject) { The double promise pattern seems unnecessary. you can do this instead, i think: if (!this._activeRequestContentPromise) { this._activeRequestContentPromise = new Promise(function(resolve, reject) { if (!this.canRequestContentFromBackend()) return Promise.reject(); return this.requestContentFromBackend().then(this._processContent.bind(this)); } } This will return the promise that's fulfilled after _processContent has run, after the SourceCode instance has been updated with the payload. _processContent already resolves with the parameters object, so it would be passed on to any clients that .then() the promise from requestContent(). I have no strong feelings about whether to return Promise.resolve() or Promise.reject() in the case where there's nothing to fetch.

Created attachment 241823 [details] [PATCH] updated WIP.

Comment on attachment 241823 [details] [PATCH] updated WIP. View in context: https://bugs.webkit.org/attachment.cgi?id=241823&action=review Looking almost ready to land.. have you tested the affected functions manually? > Source/WebInspectorUI/UserInterface/Models/Resource.js:615 > if (this.canRequestContentFromBackend()) You might as well do this unconditionally, since it will just fill in the promise slot with a rejected promise if not possible. I'm assuming that SourceCode.canRequestContentFromBackend() is invariant in all subclasses, right? If not, then we would need to think about an invalidation strategy. (If it is invariant, then we can make this clearer by storing .canRequestContentFromBackend on the constructor, then look up this.constuctor.canRequestContentFromBackend. However, we haven't done that in many other places, so maybe a comment on the SourceCode method would be better.) > Source/WebInspectorUI/UserInterface/Models/Resource.js:672 > + this.requestContent().then(function() { maybe do this? .then(function(content) { image.src = content.sourceCode.contentURL; }) > Source/WebInspectorUI/UserInterface/Models/Script.js:158 > + this.requestContent().then(function(parameters) { nice. > Source/WebInspectorUI/UserInterface/Models/SourceCode.js:130 > + this._activeRequestContentPromise = new Promise(function(resolve, reject) { Even simpler: SourceCode.processContent returns a resolved promise, so we can do: if (!this.canRequestContentFromBackend()) this._activeRequestContentPromise = Promise.reject(...); else this._activeRequestContentPromise = this.requestContentFromBackend().then(this._processContent.bind(this)); > Source/WebInspectorUI/UserInterface/Models/SourceCode.js:-201 > - requestContentFromBackendIfNeeded: function() wohooo > Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:136 > + return NetworkAgent.loadResource.promise(frameIdentifier, this.url).then(sourceMapResourceLoaded.bind(this)); // then sourceMapResourceLoaded.bind(this) what's the comment for? > Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:96 > if (this.resource.failed) { should this be just 'resource'?

Comment on attachment 241823 [details] [PATCH] updated WIP. View in context: https://bugs.webkit.org/attachment.cgi?id=241823&action=review >> Source/WebInspectorUI/UserInterface/Models/Resource.js:615 >> if (this.canRequestContentFromBackend()) > > You might as well do this unconditionally, since it will just fill in the promise slot with a rejected promise if not possible. > > I'm assuming that SourceCode.canRequestContentFromBackend() is invariant in all subclasses, right? If not, then we would need to think about an invalidation strategy. > > (If it is invariant, then we can make this clearer by storing .canRequestContentFromBackend on the constructor, then look up this.constuctor.canRequestContentFromBackend. However, we haven't done that in many other places, so maybe a comment on the SourceCode method would be better.) It's variant. Resource, SourceMapResource, and CSSStyleSheet look for different things. This check could be done however in requestContentFromBackend() which is similarly variant across these types. >> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:130 >> + this._activeRequestContentPromise = new Promise(function(resolve, reject) { > > Even simpler: SourceCode.processContent returns a resolved promise, so we can do: > > if (!this.canRequestContentFromBackend()) > this._activeRequestContentPromise = Promise.reject(...); > else > this._activeRequestContentPromise = this.requestContentFromBackend().then(this._processContent.bind(this)); Ah! Nice. I like. >> Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:136 >> + return NetworkAgent.loadResource.promise(frameIdentifier, this.url).then(sourceMapResourceLoaded.bind(this)); // then sourceMapResourceLoaded.bind(this) > > what's the comment for? It was to remind myself to add a then somewhere for sourceMapResourceLoaded. I then realized that it was an inline function. Aaaaaaaand then I left it in. Will just go ahead and clean that up. >> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:96 >> if (this.resource.failed) { > > should this be just 'resource'? Mmmmm, so there's Resource#markAsFailed() which sets this. This is used in FrameResourceManager on a stored resource, and this is used in SourceMapResource on itself.

Comment on attachment 241823 [details] [PATCH] updated WIP. View in context: https://bugs.webkit.org/attachment.cgi?id=241823&action=review >>> Source/WebInspectorUI/UserInterface/Models/Resource.js:615 >>> if (this.canRequestContentFromBackend()) >> >> You might as well do this unconditionally, since it will just fill in the promise slot with a rejected promise if not possible. >> >> I'm assuming that SourceCode.canRequestContentFromBackend() is invariant in all subclasses, right? If not, then we would need to think about an invalidation strategy. >> >> (If it is invariant, then we can make this clearer by storing .canRequestContentFromBackend on the constructor, then look up this.constuctor.canRequestContentFromBackend. However, we haven't done that in many other places, so maybe a comment on the SourceCode method would be better.) > > It's variant. Resource, SourceMapResource, and CSSStyleSheet look for different things. This check could be done however in requestContentFromBackend() which is similarly variant across these types. Well, it definitely changes per subclass. :) I was asking whether its result changes over time. >>> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:96 >>> if (this.resource.failed) { >> >> should this be just 'resource'? > > Mmmmm, so there's Resource#markAsFailed() which sets this. This is used in FrameResourceManager on a stored resource, and this is used in SourceMapResource on itself. Ah, ok. If it's not used, you should remove the local variable. It was only named before so that the argument list was accurate.

Comment on attachment 241823 [details] [PATCH] updated WIP. View in context: https://bugs.webkit.org/attachment.cgi?id=241823&action=review >>>> Source/WebInspectorUI/UserInterface/Models/Resource.js:615 >>>> if (this.canRequestContentFromBackend()) >>> >>> You might as well do this unconditionally, since it will just fill in the promise slot with a rejected promise if not possible. >>> >>> I'm assuming that SourceCode.canRequestContentFromBackend() is invariant in all subclasses, right? If not, then we would need to think about an invalidation strategy. >>> >>> (If it is invariant, then we can make this clearer by storing .canRequestContentFromBackend on the constructor, then look up this.constuctor.canRequestContentFromBackend. However, we haven't done that in many other places, so maybe a comment on the SourceCode method would be better.) >> >> It's variant. Resource, SourceMapResource, and CSSStyleSheet look for different things. This check could be done however in requestContentFromBackend() which is similarly variant across these types. > > Well, it definitely changes per subclass. :) I was asking whether its result changes over time. OH! of course.

Regarding landability, so far source loads as expected but I am testing in more detail. I have no idea yet if source maps work either. I'm unsure of my changes there but I think I got it right....

Created attachment 242001 [details] [PATCH] Very close, one refresh bug. Here is an update that works well except for that refreshing the page causes an error in the inspector in SourceCodeTextEditor.js because of these changes. Looking into that. I think it's really close.

Created attachment 242962 [details] [PATCH] fix for review. This patch has been tested with source maps, resources that doesn't exist on the server, page navigations, etc. It is, as best as I can tell, complete.

Created attachment 242968 [details] [PATCH] fix for review ver 2. Forgot to remove an unnecessary line of code.

Comment on attachment 242968 [details] [PATCH] fix for review ver 2. View in context: https://bugs.webkit.org/attachment.cgi?id=242968&action=review Looks good except for the listener leaking. Please fix that and post another patch or land yourself. > Source/WebInspectorUI/ChangeLog:11 > + if an content request error needs to be handled. Fix a bug where the appropriate error message for an if an -> if a > Source/WebInspectorUI/UserInterface/Models/Resource.js:-561 > - // There is no request identifier or frame to request content from. Return false to cause the I would preserve the first sentence of the comment. > Source/WebInspectorUI/UserInterface/Models/Resource.js:689 > + this.addEventListener(WebInspector.Resource.Event.LoadingDidFinish, resolve); This will leak the promise because the WebInspector.Resource keep a hard reference to the listener. You need to use WebInspector.EventListener; see DebuggerManager's resume, stepOver, etc to see how I avoided this. > Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:34 > + //this._contentRequested = false; Oops > Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:81 > Oops

Comment on attachment 242968 [details] [PATCH] fix for review ver 2. View in context: https://bugs.webkit.org/attachment.cgi?id=242968&action=review > Source/WebInspectorUI/UserInterface/Models/CSSStyleSheet.js:129 > + return Promise.reject({ message: "There is no identifier to request content with." }); Ditto. > Source/WebInspectorUI/UserInterface/Models/Resource.js:559 > + return Promise.reject({ > + message: "Content request failed." > + }); Ditto. > Source/WebInspectorUI/UserInterface/Models/Resource.js:682 > + if (this._failed) This needs braces since the body of the condition is multi-line. > Source/WebInspectorUI/UserInterface/Models/Resource.js:685 > + return Promise.reject({ > + message: "An error occurred trying to load the resource." > + }); Shouldn't this be an Error object? Per http://trac.webkit.org/wiki/WebInspectorCodingStyleGuide#PromiseGotchas. > Source/WebInspectorUI/UserInterface/Models/Resource.js:687 > + if (!this._finishThenRequestContentPromise) Ditto. > Source/WebInspectorUI/UserInterface/Models/Script.js:118 > + return Promise.reject({ message: "There is no identifier to request content with." }); Ditto. > Source/WebInspectorUI/UserInterface/Models/SourceCode.js:186 > + return Promise.reject({ > + message: "Needs to be implemented by a subclass." > + }); Ditto. > Source/WebInspectorUI/UserInterface/Models/SourceMapResource.js:127 > + return Promise.reject({ message: "error: no NetworkAgent.loadResource" }); Ditto.

Comment on attachment 242968 [details] [PATCH] fix for review ver 2. View in context: https://bugs.webkit.org/attachment.cgi?id=242968&action=review >> Source/WebInspectorUI/UserInterface/Models/Resource.js:689 >> + this.addEventListener(WebInspector.Resource.Event.LoadingDidFinish, resolve); > > This will leak the promise because the WebInspector.Resource keep a hard reference to the listener. You need to use WebInspector.EventListener; see DebuggerManager's resume, stepOver, etc to see how I avoided this. Is that not ok? The idea is to have a promise that is fulfilled that is returned after the content has been successfully requested. Seems like with your solution I'll have to go back to something like this._contentLoaded = true; somewhere.

Comment on attachment 242968 [details] [PATCH] fix for review ver 2. View in context: https://bugs.webkit.org/attachment.cgi?id=242968&action=review >>> Source/WebInspectorUI/UserInterface/Models/Resource.js:689 >>> + this.addEventListener(WebInspector.Resource.Event.LoadingDidFinish, resolve); >> >> This will leak the promise because the WebInspector.Resource keep a hard reference to the listener. You need to use WebInspector.EventListener; see DebuggerManager's resume, stepOver, etc to see how I avoided this. > > Is that not ok? The idea is to have a promise that is fulfilled that is returned after the content has been successfully requested. Seems like with your solution I'll have to go back to something like this._contentLoaded = true; somewhere. The current design of caching the promise is totally fine. What I'm concerned about is the promise never being GC'd even if we navigate to a different inspected page. In that case I would expect the promises from the old page to be eventually GC'd. If the promise is bound to |this| then it seems the Resource won't get GC'd either, no?

Comment on attachment 242968 [details] [PATCH] fix for review ver 2. View in context: https://bugs.webkit.org/attachment.cgi?id=242968&action=review >>>> Source/WebInspectorUI/UserInterface/Models/Resource.js:689 >>>> + this.addEventListener(WebInspector.Resource.Event.LoadingDidFinish, resolve); >>> >>> This will leak the promise because the WebInspector.Resource keep a hard reference to the listener. You need to use WebInspector.EventListener; see DebuggerManager's resume, stepOver, etc to see how I avoided this. >> >> Is that not ok? The idea is to have a promise that is fulfilled that is returned after the content has been successfully requested. Seems like with your solution I'll have to go back to something like this._contentLoaded = true; somewhere. > > The current design of caching the promise is totally fine. > > What I'm concerned about is the promise never being GC'd even if we navigate to a different inspected page. > In that case I would expect the promises from the old page to be eventually GC'd. If the promise is bound to |this| then it seems the Resource won't get GC'd either, no? Hmm, I don't think there is a leak here. "this.addEventListener" is an instance adding an event listener. Yes, there is a cycle between the resource / promise / and event listener, but that is fine as long as the objects cannot be referenced from the rest of the heap (GC roots), then the set of objects will get GC'd. Normally the leaks we worry about are event listeners on constructors (WebInspector.Resource.addEventListener) not instances. Because the constructor will never go away, and it will forever hold onto its listeners. Instances however, will go away. In this case, resources are destroyed when you navigate.

Created attachment 243586 [details] [PATCH] fix for review ver 3. Changes addressed. Also fixes bug with viewing source with debugger statements and other Script content.

Comment on attachment 243586 [details] [PATCH] fix for review ver 3. Clearing flags on attachment: 243586 Committed r177791: <http://trac.webkit.org/changeset/177791>

All reviewed patches have been landed. Closing bug.