Bug 204086

This is similar to https://bugs.webkit.org/show_bug.cgi?id=203722. The problem is now SourceCode.currentRevision will always create a copy of the original revision. In the scenario described above scripts content is fetched before the document resource content. When resolving offsets of the inline scripts from SourceCodeTextEditor._getAssociatedScript(position) to access current content it does the following: inlineScript.range.resolveOffsets(this._sourceCode.content); which will clone original revision. Now the new revision of the main resource has empty content and is different from the original one. When resource content is received it will be set to the original revision of the resource but SourceCodeTextEditor._contentAvailable will retrieve it from the current revision of the source code which will be empty.

Created attachment 383368 [details] Patch

Comment on attachment 383368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383368&action=review r=me > Source/WebInspectorUI/UserInterface/Models/SourceCode.js:72 > + get editableRevision() Please move this after `set currentRevision` so we don't split up a get-set pair. > Source/WebInspectorUI/UserInterface/Models/SourceCode.js:190 > + if (revision !== this._currentRevision) Let's add a `console.assert(revision === this._currentRevision);` before this. I don't think we ever want to update non-current revisions. > Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:215 > + // The view maybe populated with inline scripts content by the time resource > + // content arrives. SourceCodeTextEditor will handle that. > + if (this._hasContent()) > + return; Why is this needed? I tried doing some testing myself and it seems to work fine. Do you have a specific test case where this is needed? If so, what exactly is "wrong" when this isn't included? > Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:297 > + let revision = localResourceOverride.localResource.editableRevision; Aside: we should probably move this inside the callback, in case the revision changes in between.

Comment on attachment 383368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383368&action=review >> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:72 >> + get editableRevision() > > Please move this after `set currentRevision` so we don't split up a get-set pair. Done. >> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:190 >> + if (revision !== this._currentRevision) > > Let's add a `console.assert(revision === this._currentRevision);` before this. I don't think we ever want to update non-current revisions. Done. >> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:215 >> + return; > > Why is this needed? I tried doing some testing myself and it seems to work fine. Do you have a specific test case where this is needed? If so, what exactly is "wrong" when this isn't included? When execution breaks in inline script the content will be first assembled from scripts coming from the debugger. When main resource loading finishes the content will be reset from the main resource. This can be easily reproduced with inline breakpoints. >> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:297 >> + let revision = localResourceOverride.localResource.editableRevision; > > Aside: we should probably move this inside the callback, in case the revision changes in between. Done. Given that there is at most 2 revisions ever this should not matter as the one created by editableRevision is guaranteed to stay the latest.

Created attachment 383975 [details] Patch for landing

Comment on attachment 383975 [details] Patch for landing Clearing flags on attachment: 383975 Committed r252704: <https://trac.webkit.org/changeset/252704>

All reviewed patches have been landed. Closing bug.