Web Inspector: REGRESSION(r250618): main resource view is empty when pausing on inline 'debugger' statement

[Yury Semikhatsky]

Created attachment 383304 [details]
test page

Steps to reproduce:
1. Load attached page.
2. Open Web Inspector.
3. Reload inspected page.

Expected:
Execution is paused on the debugger; statement and main resource is displayed.

Actual:
Main resource view is blank.

resource:///org/webkit/inspector/UserInterface/Views/ResourceContentView.js:213:23: CONSOLE ERROR

is printed to the console.

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Created attachment 383368 [details]
Patch

[Devin Rousso]

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.

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Created attachment 383975 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 383975 [details]
Patch for landing

Clearing flags on attachment: 383975

Committed r252704: <https://trac.webkit.org/changeset/252704>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.