Bug 138665

Summary:

Web Inspector: Agent commands do not actually return a promise when expected

Product:

WebKit

Reporter:

Brian Burg <burg>

Component:

Web Inspector

Assignee:

Blaze Burg <bburg>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, buildbot, graouts, joepeck, jonowells, rniwa, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

147067, 147093

Attachments:

Description	Flags
WIP	none
Proposed Fix	timothy: review+, buildbot: commit-queue-
Archive of layout-test-results from ews106 for mac-mavericks-wk2	none
Archive of layout-test-results from ews101 for mac-mavericks	none

Brian Burg

Reported 2014-11-12 13:29:28 PST

It seems that LayoutTests/inspector/protocol-return-promise.html is buggy, masking the error. It's only found now because Jono tried to .then on an agent command without .promise(). We don't actually return a promise from InspectorBackend.Command.create.(fn callable). That's because invokeWithArguments doesn't return a promise either- we need to convert the manual callback to a promise in (fn callable) like we do in InspectorBackend.Command.promise.

Attachments
WIP (14.07 KB, patch) 2015-08-11 07:36 PDT, Blaze Burg	no flags	Details Formatted Diff Diff
Proposed Fix (31.54 KB, patch) 2015-08-11 11:08 PDT, Brian Burg	timothy: review+ buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (631.62 KB, application/zip) 2015-08-11 11:45 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews101 for mac-mavericks (542.48 KB, application/zip) 2015-08-11 11:58 PDT, Build Bot	no flags	Details
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2014-11-12 13:29:42 PST

<rdar://problem/18960020>

Blaze Burg

Comment 2 2015-08-11 07:36:16 PDT

Created attachment 258716 [details] WIP

Timothy Hatcher

Comment 3 2015-08-11 11:06:22 PDT

Comment on attachment 258716 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=258716&action=review Looks good. > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:154 > + ++this._pendingResponsesCount; > + let sequenceId = this._lastSequenceId++; Newline. > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:196 > + console.assert(this._callbackData.has(sequenceId), sequenceId, this._callbackData); > + let responseData = this._callbackData.take(sequenceId); I like to put newlines after asserts like this. Or move the other let statements up. > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:199 > + var processingStartTime; let > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:208 > + var processingDuration = Date.now() - processingStartTime; let > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:224 > + for (var parameterName of command.replySignature) let > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:448 > + if (typeof callback === "function") > + return this._instance._backend._sendCommandToBackendWithCallback(instance, commandArguments, callback); > + else > + return this._instance._backend._sendCommandToBackendExpectingPromise(instance, commandArguments); No need for the else. > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:504 > + if (callback) > + return instance._backend._sendCommandToBackendWithCallback(instance, parameters, callback); > + else > + return instance._backend._sendCommandToBackendExpectingPromise(instance, parameters); No need for the else.

Brian Burg

Comment 4 2015-08-11 11:08:21 PDT

Created attachment 258727 [details] Proposed Fix

Joseph Pecoraro

Comment 5 2015-08-11 11:13:34 PDT

Comment on attachment 258727 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=258727&action=review I haven't looked at this in detail yet, so leaving r? > Source/WebInspectorUI/ChangeLog:63 > +2015-08-11 Brian Burg <bburg@apple.com> > + > + Web Inspector: Agent commands do not actually return a promise when expected > + https://bugs.webkit.org/show_bug.cgi?id=138665 Double ChangeLog. > Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:113 > + return !this.element.querySelector("." + "indeterminate-progress-spinner"); This seems like an ideal place to actually have had the variable. But, if we are dropping the variable, then no need for string concatenation here, just do ".foo" as one string.

Timothy Hatcher

Comment 6 2015-08-11 11:15:22 PDT

Comment on attachment 258727 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=258727&action=review > Source/WebInspectorUI/ChangeLog:65 > +2015-08-11 Brian Burg <bburg@apple.com> > + > + Web Inspector: Agent commands do not actually return a promise when expected > + https://bugs.webkit.org/show_bug.cgi?id=138665 > + > + Reviewed by NOBODY (OOPS!). Double ChangeLog. > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:95 > + if (this._pendingResponses.size === 0) ! here. > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:159 > + responseData.sendRequestTime = Date.now(); Should we use performance.now()? > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:448 > + return instance._backend._sendCommandToBackendWithCallback(instance, commandArguments, callback); > + else > + return instance._backend._sendCommandToBackendExpectingPromise(instance, commandArguments); No else. > Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:504 > + return instance._backend._sendCommandToBackendWithCallback(instance, parameters, callback); > + else > + return instance._backend._sendCommandToBackendExpectingPromise(instance, parameters); No else. > Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:113 > - return !this.element.querySelector("." + WebInspector.IndeterminateProgressSpinner.StyleClassName); > + return !this.element.querySelector("." + "indeterminate-progress-spinner"); Land separate.

Blaze Burg

Comment 7 2015-08-11 11:31:32 PDT

Comment on attachment 258727 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=258727&action=review >> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:159 >> + responseData.sendRequestTime = Date.now(); > > Should we use performance.now()? AFAIK, it's not enabled for Mavericks, so we'd want to polyfill it. A bug exists for this already: https://bugs.webkit.org/show_bug.cgi?id=135467 >> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:448 >> + return instance._backend._sendCommandToBackendExpectingPromise(instance, commandArguments); > > No else. Er, the if branch should not return anything. >> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:504 >> + return instance._backend._sendCommandToBackendExpectingPromise(instance, parameters); > > No else. As above. >>> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:113 >>> + return !this.element.querySelector("." + "indeterminate-progress-spinner"); >> >> This seems like an ideal place to actually have had the variable. >> >> But, if we are dropping the variable, then no need for string concatenation here, just do ".foo" as one string. > > Land separate. Filed: https://bugs.webkit.org/show_bug.cgi?id=147886

Build Bot

Comment 8 2015-08-11 11:45:41 PDT

Comment on attachment 258727 [details] Proposed Fix Attachment 258727 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/45314 New failing tests: inspector/protocol/inspector-backend-invocation-return-value.html

Build Bot

Comment 9 2015-08-11 11:45:43 PDT

Created attachment 258730 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Build Bot

Comment 10 2015-08-11 11:58:53 PDT

Comment on attachment 258727 [details] Proposed Fix Attachment 258727 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/45345 New failing tests: inspector/protocol/inspector-backend-invocation-return-value.html

Build Bot

Comment 11 2015-08-11 11:58:55 PDT

Created attachment 258733 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5

Blaze Burg

Comment 12 2015-08-11 12:52:22 PDT

(In reply to comment #11) > Created attachment 258733 [details] > Archive of layout-test-results from ews101 for mac-mavericks > > The attached test failures were seen while running run-webkit-tests on the > mac-ews. > Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5 Oops, forgot to include changes to the test in the <body>. Will update before landing.

Blaze Burg

Comment 13 2015-08-11 13:15:40 PDT

Committed r188283: <http://trac.webkit.org/changeset/188283>

Note You need to log in before you can comment on or make changes to this bug.