New issue
Advanced search Search tips

Issue 648275 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 648276

Blocking:
issue 634140



Sign in to add a comment

Isolate ExtensionFunction responses and errors

Project Member Reported by rdevlin....@chromium.org, Sep 19 2016

Issue description

The fact that we expose results_ and error_ directly to ExtensionFunction implementations has caused a lot of confusion and inconsistent results.  These should instead be set implicitly by the return values of Run() or calls to Respond(), to avoid having contradictory results.
 
Blockedon: 648276
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eedb95b1a034bb392ed83e84ee690395a0db3e27

commit eedb95b1a034bb392ed83e84ee690395a0db3e27
Author: rdevlin.cronin <rdevlin.cronin@chromium.org>
Date: Wed Sep 21 02:41:17 2016

[Extensions] Consolidate ExtensionFunction::SendResponse()s

De-virtualize ExtensionFunction::SendResponse() and contain all work in
a single SendResponseImpl()* method, which is private to
ExtensionFunction and can only be invoked through the Respond() methods.
This is a first step in isolating results_, error_, and the response
invocation in order to ensure consistent and clear results from
functions.

Update UIThreadExtensionFunction implementations that used SendResponse()
rather than Respond().

For compatibility, introduce SendResponse() methods on legacy
ExtensionFunction classes (ChromeUIThreadExtensionFunction and
AsyncExtensionFunction) which wrap around Respond(). As functions are
converted, this wrapper will go away.

Also remove the unused ExtensionFunction::OnRespondingLater().

*SendResponseImpl() instead of SendResponse() so that we don't have to
rename existing calls in legacy functions.

BUG=648275

Review-Url: https://codereview.chromium.org/2351823004
Cr-Commit-Position: refs/heads/master@{#419955}

[modify] https://crrev.com/eedb95b1a034bb392ed83e84ee690395a0db3e27/chrome/browser/chromeos/extensions/file_manager/private_api_base.cc
[modify] https://crrev.com/eedb95b1a034bb392ed83e84ee690395a0db3e27/chrome/browser/chromeos/extensions/file_manager/private_api_base.h
[modify] https://crrev.com/eedb95b1a034bb392ed83e84ee690395a0db3e27/chrome/browser/extensions/api/file_system/file_system_api.cc
[modify] https://crrev.com/eedb95b1a034bb392ed83e84ee690395a0db3e27/chrome/browser/extensions/api/terminal/terminal_private_api.cc
[modify] https://crrev.com/eedb95b1a034bb392ed83e84ee690395a0db3e27/chrome/browser/extensions/api/webstore_widget_private/webstore_widget_private_api.cc
[modify] https://crrev.com/eedb95b1a034bb392ed83e84ee690395a0db3e27/chrome/browser/extensions/chrome_extension_function.cc
[modify] https://crrev.com/eedb95b1a034bb392ed83e84ee690395a0db3e27/chrome/browser/extensions/chrome_extension_function.h
[modify] https://crrev.com/eedb95b1a034bb392ed83e84ee690395a0db3e27/extensions/browser/api/printer_provider_internal/printer_provider_internal_api.cc
[modify] https://crrev.com/eedb95b1a034bb392ed83e84ee690395a0db3e27/extensions/browser/extension_function.cc
[modify] https://crrev.com/eedb95b1a034bb392ed83e84ee690395a0db3e27/extensions/browser/extension_function.h
[modify] https://crrev.com/eedb95b1a034bb392ed83e84ee690395a0db3e27/extensions/browser/quota_service_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/756d84a652255bad6feee508c1f9f6b8b9e1c4c5

commit 756d84a652255bad6feee508c1f9f6b8b9e1c4c5
Author: rdevlin.cronin <rdevlin.cronin@chromium.org>
Date: Sat Oct 01 01:58:38 2016

[Extensions] Isolate ExtensionFunction results_ and error_

ExtensionFunction results and errors should be set exclusively through
the responses of the function implementations in order to reduce the
likelihood of malformed or inconsistent responses.

Move results_ and error_ to be private members of ExtensionFunction. For
backwards compatibility, surface new members on legacy function
implementations (AsyncExtensionFunction, Chrome*ExtensionFunction) that
are then used in the responses of the functions.

As we continue to migrate away from these legacy implementations, we can
continue to lock down the response values.

BUG=648275

Review-Url: https://codereview.chromium.org/2360073002
Cr-Commit-Position: refs/heads/master@{#422270}

[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/browser/extensions/api/browsing_data/browsing_data_api.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/browser/extensions/api/inline_install_private/inline_install_private_api.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/browser/extensions/api/input_ime/input_ime_api.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/browser/extensions/api/management/chrome_management_api_delegate.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/browser/extensions/api/management/chrome_management_api_delegate.h
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/browser/extensions/api/preference/preference_api.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/browser/extensions/api/tabs/tabs_api.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/browser/extensions/chrome_extension_function.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/browser/extensions/chrome_extension_function.h
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/test/data/extensions/api_test/permissions/file_access_no/background.js
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/test/data/extensions/api_test/permissions/host_subsets/background.js
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/chrome/test/data/extensions/api_test/permissions/optional_retain_gesture/background.js
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/extensions/browser/api/execute_code_function.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/extensions/browser/api/guest_view/web_view/web_view_internal_api.h
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/extensions/browser/api/hid/hid_api.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/extensions/browser/api/management/management_api.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/extensions/browser/api/management/management_api_delegate.h
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/extensions/browser/api/web_request/web_request_api.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/extensions/browser/extension_function.cc
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/extensions/browser/extension_function.h
[modify] https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5/extensions/browser/quota_service_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fd7bb627894ab5fc1415c90f579dba779fddc70f

commit fd7bb627894ab5fc1415c90f579dba779fddc70f
Author: rdevlin.cronin <rdevlin.cronin@chromium.org>
Date: Mon Oct 03 23:14:46 2016

[Extensions] Remove ExtensionFunction::SetError()

An extension function error should be set only through the use of the
response (i.e., ExtensionFunction::Error()), and setting it directly
can lead to confusion and incorrect results. Remove
ExtensionFunction::SetError(). Other subclasses (like
AsyncExtensionFunction) still have accessors, but will be updated as
they are converted.

Also disable two more input IME API tests that were always failing, but
silently passed because of passing success despite having an error.

BUG=648275

Review-Url: https://codereview.chromium.org/2386823002
Cr-Commit-Position: refs/heads/master@{#422589}

[modify] https://crrev.com/fd7bb627894ab5fc1415c90f579dba779fddc70f/chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
[modify] https://crrev.com/fd7bb627894ab5fc1415c90f579dba779fddc70f/chrome/browser/extensions/api/input_ime/input_ime_api.cc
[modify] https://crrev.com/fd7bb627894ab5fc1415c90f579dba779fddc70f/chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc
[modify] https://crrev.com/fd7bb627894ab5fc1415c90f579dba779fddc70f/chrome/browser/extensions/api/sessions/sessions_api.cc
[modify] https://crrev.com/fd7bb627894ab5fc1415c90f579dba779fddc70f/chrome/browser/extensions/api/tabs/tabs_api.cc
[modify] https://crrev.com/fd7bb627894ab5fc1415c90f579dba779fddc70f/chrome/browser/extensions/api/tabs/windows_util.cc
[modify] https://crrev.com/fd7bb627894ab5fc1415c90f579dba779fddc70f/chrome/browser/extensions/api/tabs/windows_util.h
[modify] https://crrev.com/fd7bb627894ab5fc1415c90f579dba779fddc70f/chrome/browser/extensions/chrome_extension_function.cc
[modify] https://crrev.com/fd7bb627894ab5fc1415c90f579dba779fddc70f/chrome/browser/extensions/chrome_extension_function.h
[modify] https://crrev.com/fd7bb627894ab5fc1415c90f579dba779fddc70f/chrome/test/data/extensions/api_test/input_ime/background.js
[modify] https://crrev.com/fd7bb627894ab5fc1415c90f579dba779fddc70f/extensions/browser/extension_function.cc
[modify] https://crrev.com/fd7bb627894ab5fc1415c90f579dba779fddc70f/extensions/browser/extension_function.h
[modify] https://crrev.com/fd7bb627894ab5fc1415c90f579dba779fddc70f/extensions/browser/quota_service_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fe9846c68f20d9ec068aa960e7700b8c3d9a8d19

commit fe9846c68f20d9ec068aa960e7700b8c3d9a8d19
Author: lazyboy <lazyboy@chromium.org>
Date: Fri Jan 20 00:57:12 2017

Remove usage of AsyncExtensionFunction::results_ in system_network_api.cc

Convert relevant classes to directly inherit from UIThreadExtensionFunction.

BUG=648275

Review-Url: https://codereview.chromium.org/2646703002
Cr-Commit-Position: refs/heads/master@{#444903}

[modify] https://crrev.com/fe9846c68f20d9ec068aa960e7700b8c3d9a8d19/extensions/browser/api/system_network/system_network_api.cc
[modify] https://crrev.com/fe9846c68f20d9ec068aa960e7700b8c3d9a8d19/extensions/browser/api/system_network/system_network_api.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/30abd07e271e93f99b70745a08b360793be779cc

commit 30abd07e271e93f99b70745a08b360793be779cc
Author: lazyboy <lazyboy@chromium.org>
Date: Mon Jan 23 22:12:32 2017

Introduce AlreadyResponded() response action from ExtensionFunction.

Currently ExtensionFunction::Run has to either:
call RespondNow() to synchronously respond or
call RespondLater() to asynchronously Respond() later point in time after
  Run() finishes.

There are some asynchronous ExtensionFunction implementations where,
due to how base:: callbacks are used in them, it can be difficult to
pick either one of the two responses above. This happens when a callback
used in Run() can sometimes synchronously fire and if the callback
calls Respond(), then returning RespondLater() from Run() would be
a wrong choice.

This CL provides AlreadyResponded() response action for those cases.
While returning response from Run(), the ExtensionFunction can check
whether it has already responded and return AlreadyResponded(). Otherwise,
it can return RespondLater().

This CL removes some usages of AsyncExtensionFunction::results_ in turn.
This CL also converts those classes to directly inherit from
UIThreadExtensionFunction.

BUG=648275

Review-Url: https://codereview.chromium.org/2612873004
Cr-Commit-Position: refs/heads/master@{#445507}

[modify] https://crrev.com/30abd07e271e93f99b70745a08b360793be779cc/extensions/browser/api/management/management_api.cc
[modify] https://crrev.com/30abd07e271e93f99b70745a08b360793be779cc/extensions/browser/api/management/management_api.h
[modify] https://crrev.com/30abd07e271e93f99b70745a08b360793be779cc/extensions/browser/api/networking_private/networking_private_api.cc
[modify] https://crrev.com/30abd07e271e93f99b70745a08b360793be779cc/extensions/browser/api/networking_private/networking_private_api.h
[modify] https://crrev.com/30abd07e271e93f99b70745a08b360793be779cc/extensions/browser/api/system_storage/system_storage_api.cc
[modify] https://crrev.com/30abd07e271e93f99b70745a08b360793be779cc/extensions/browser/api/system_storage/system_storage_api.h
[modify] https://crrev.com/30abd07e271e93f99b70745a08b360793be779cc/extensions/browser/extension_function.cc
[modify] https://crrev.com/30abd07e271e93f99b70745a08b360793be779cc/extensions/browser/extension_function.h

Project Member

Comment 7 by bugdroid1@chromium.org, Jan 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c9106745093e61a44cd3a0f6125dca4ddf8708ca

commit c9106745093e61a44cd3a0f6125dca4ddf8708ca
Author: lazyboy <lazyboy@chromium.org>
Date: Wed Jan 25 22:09:35 2017

Clean up param validation in browsing_data_api.cc

Instead of manually setting and reading ExtensionFunction::bad_message_,
use EXTENSION_FUNCTION_VALIDATE from ::Run*.

Remove ExtensionFunction::bad_message() since it will no longer be used.

BUG=648275
Test=None, refactor change.

Review-Url: https://codereview.chromium.org/2650343005
Cr-Commit-Position: refs/heads/master@{#446139}

[modify] https://crrev.com/c9106745093e61a44cd3a0f6125dca4ddf8708ca/chrome/browser/extensions/api/browsing_data/browsing_data_api.cc
[modify] https://crrev.com/c9106745093e61a44cd3a0f6125dca4ddf8708ca/chrome/browser/extensions/api/browsing_data/browsing_data_api.h
[modify] https://crrev.com/c9106745093e61a44cd3a0f6125dca4ddf8708ca/extensions/browser/extension_function.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9bfcba8467532d955f1178b70c4e3b88e6e5f23e

commit 9bfcba8467532d955f1178b70c4e3b88e6e5f23e
Author: lazyboy <lazyboy@chromium.org>
Date: Sat Jan 28 02:08:55 2017

Move rest of param validation in browsing_data_api.cc to ::RunAsync.

It's a bit cleaner to call EXTENSION_FUNCTION_VALIDATE directly from ::Run*.
(ab)using EXTENSION_FUNCTION_VALIDATE's implicit bool conversion as
return value for non-Run* functions is discouraged.

BUG=648275
Test=None

Review-Url: https://codereview.chromium.org/2654113003
Cr-Commit-Position: refs/heads/master@{#446883}

[modify] https://crrev.com/9bfcba8467532d955f1178b70c4e3b88e6e5f23e/chrome/browser/extensions/api/browsing_data/browsing_data_api.cc
[modify] https://crrev.com/9bfcba8467532d955f1178b70c4e3b88e6e5f23e/chrome/browser/extensions/api/browsing_data/browsing_data_api.h

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df7686e843360e2ae174bda45648708b59653830

commit df7686e843360e2ae174bda45648708b59653830
Author: lazyboy <lazyboy@chromium.org>
Date: Fri Feb 17 19:52:27 2017

history_api -> UIThreadExtensionFunction.

This removes direct usage of AsyncExtensionFunction::error_/results_.

BUG=648275
Test=None, compile.

Review-Url: https://codereview.chromium.org/2669423004
Cr-Commit-Position: refs/heads/master@{#451358}

[modify] https://crrev.com/df7686e843360e2ae174bda45648708b59653830/chrome/browser/extensions/api/history/history_api.cc
[modify] https://crrev.com/df7686e843360e2ae174bda45648708b59653830/chrome/browser/extensions/api/history/history_api.h

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/80b39f01869d523307a6b494167fdf4f3e533b53

commit 80b39f01869d523307a6b494167fdf4f3e533b53
Author: lazyboy <lazyboy@chromium.org>
Date: Tue Apr 04 22:51:03 2017

Make AppWindowCreateFunction UIThreadExtensionFunction.

AsyncExtensionFunction is deprecated.

Also rename app_window tests to *AppWindowApiTest instead of
*PlatformAppBrowserTest.

BUG=648275
Test=Expect no visible changes.

Review-Url: https://codereview.chromium.org/2789413003
Cr-Commit-Position: refs/heads/master@{#461876}

[modify] https://crrev.com/80b39f01869d523307a6b494167fdf4f3e533b53/extensions/browser/api/app_window/app_window_api.cc
[modify] https://crrev.com/80b39f01869d523307a6b494167fdf4f3e533b53/extensions/browser/api/app_window/app_window_api.h
[modify] https://crrev.com/80b39f01869d523307a6b494167fdf4f3e533b53/extensions/browser/api/app_window/app_window_apitest.cc

Sign in to add a comment