WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
204086
Web Inspector: REGRESSION(
r250618
): main resource view is empty when pausing on inline 'debugger' statement
https://bugs.webkit.org/show_bug.cgi?id=204086
Summary
Web Inspector: REGRESSION(r250618): main resource view is empty when pausing ...
Yury Semikhatsky
Reported
2019-11-11 14:58:22 PST
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.
Attachments
test page
(124 bytes, text/html)
2019-11-11 14:58 PST
,
Yury Semikhatsky
no flags
Details
Patch
(10.39 KB, patch)
2019-11-12 11:46 PST
,
Yury Semikhatsky
no flags
Details
Formatted Diff
Diff
Patch for landing
(10.57 KB, patch)
2019-11-20 11:33 PST
,
Yury Semikhatsky
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Yury Semikhatsky
Comment 1
2019-11-12 10:19:39 PST
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
Comment 2
2019-11-12 11:46:29 PST
Created
attachment 383368
[details]
Patch
Devin Rousso
Comment 3
2019-11-20 10:53:53 PST
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 4
2019-11-20 11:32:28 PST
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
Comment 5
2019-11-20 11:33:42 PST
Created
attachment 383975
[details]
Patch for landing
WebKit Commit Bot
Comment 6
2019-11-20 13:23:43 PST
Comment on
attachment 383975
[details]
Patch for landing Clearing flags on attachment: 383975 Committed
r252704
: <
https://trac.webkit.org/changeset/252704
>
WebKit Commit Bot
Comment 7
2019-11-20 13:23:45 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug