Just encountered a 5 second delay in Feedly Notifier. 

Apparently, now Chrome additionally waits for all external images to be loaded before showing the popup. This is wrong. Previously the popup was shown immediately and the images were loaded in the background like they always do on web pages.

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

commit 8e744a465d70e2ab1bf7959a66c3c65bd3850092
Author: rdevlin.cronin <rdevlin.cronin@chromium.org>
Date: Tue Dec 06 18:40:27 2016

[Extensions] Extension Port Ids and Initialization 2.0

Extensions can open message ports to communicate between different
contexts. A port can have n different receivers, and communication can
flow bidirectionally.

Currently, this is implemented by the port having a global id, vended by
the browser process, which uniquely identifies a port (amongst all
contexts). In making port creation asynchronous ( crbug.com/642380 ), we
also added the concept of a "local id" to be used in Javascript that
identified a port uniquely in the context. This allowed us to have the
browser asynchronously message us back a global id for the port to use.

Unfortunately, this still has a number of problems:
- Asynchronous port creation doesn't work in onunload
  ( crbug.com/660706 ); this also required exposing the unload state from
  blink.
- The time-to-message is slower, since we wait on an IPC round trip
  ( crbug.com/649942 ).

Instead, construct a new PortId concept as a combination of three
members: context id (a globally-unique id for the context), port number
(a port id within that context), and whether or not the port is an
opener. This allows us to:
- Make a port ID on the renderer, thus making it entirely synchronous
  and even faster than before (we don't need the browser involved at
  all). Also without exposing knowledge of unload state.
- Identify whether the opposite end of the port was created in the same
  context (solving the issue of re-opening extension ports in a same-
  process renderer,  crbug.com/597698 ). Test added courtesy of
  rob@robwu.nl.
- Increase difficulty in sending bad IPCs since an attacker would have
  to guess another context's GUID (rather than just being able to know
  that a port with id '1' exists somewhere).

Additionally, eliminate a few hundred lines of code.

BUG= 649942 
BUG= 597698 

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

[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/chrome/browser/extensions/api/messaging/extension_message_port.cc
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/chrome/browser/extensions/api/messaging/extension_message_port.h
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/chrome/browser/extensions/api/messaging/message_service.cc
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/chrome/browser/extensions/api/messaging/message_service.h
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/chrome/browser/extensions/api/messaging/native_message_port.cc
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/chrome/browser/extensions/api/messaging/native_message_port.h
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/chrome/browser/renderer_host/chrome_extension_message_filter.cc
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/chrome/browser/renderer_host/chrome_extension_message_filter.h
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/chrome/renderer/chrome_mock_render_thread.cc
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/chrome/renderer/chrome_mock_render_thread.h
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/chrome/test/data/extensions/api_test/messaging/background_only/test.js
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/extensions/common/BUILD.gn
[add] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/extensions/common/api/messaging/port_id.cc
[add] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/extensions/common/api/messaging/port_id.h
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/extensions/common/extension_messages.h
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/extensions/renderer/dispatcher.cc
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/extensions/renderer/dispatcher.h
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/extensions/renderer/extension_frame_helper.cc
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/extensions/renderer/extension_frame_helper.h
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/extensions/renderer/extension_port.cc
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/extensions/renderer/extension_port.h
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/extensions/renderer/messaging_bindings.cc
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/extensions/renderer/messaging_bindings.h
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/third_party/WebKit/Source/web/WebDocument.cpp
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/third_party/WebKit/public/web/WebDocument.h
[modify] https://crrev.com/8e744a465d70e2ab1bf7959a66c3c65bd3850092/tools/metrics/histograms/histograms.xml

 Issue 673000  has been merged into this issue.

woxxom@, if this was caused by https://crrev.com/2331263002, it should be fixed with the patch in #6.  Can you test if you're still seeing it on canary?

re #8: definitely seeing the issue in 57.0.2949.0. Only the workaround posted in  issue 673000  fixes it: remove any LINK tags from the popup html file and insert them in js code instead. Please re-bisect.

Mmkay, so definitely not https://crrev.com/2331263002.  Let's get a real bisect here.

FWIW, the bisect above was my first time doing it.
Here's a new bisect that matches the mentioned workaround, and this time I clicked the extension icon 5-10 times to make sure I'm not imagining the delay:
418832 (good) - 418836 (bad), 55.0.2862.0
https://chromium.googlesource.com/chromium/src/+log/89809f93..6becf9c4?pretty=fuller
Suspecting r418835 "Remove EventSender from HTMLLinkElement"

cc'ing hiroshige@ per #12.

Thanks for your help on this woxxom@. :)

 Issue 657804  might be related (delay in extension related to <link> element caused by the same CL).

Passing to hiroshige@.  hiroshige@,  issue 657804  was determined to be a race condition in that particular extension; is this the case with all of the extensions that are affected by this?  Or did r418835 have an unintended performance side effect, if this is becoming drastically more common?

OK, I'll investigate.

I see a delay present in the LastPass extension as well. 

In case it's a helpful reference point, one extension that I do NOT see the popup delay on is "Salesforce Hotkeys"  https://chrome.google.com/webstore/detail/salesforce-hotkeys-beta/hkpmdgakkflkddmiffelfaokkgoamlil

Locally reproduced on Linux and confirmed that r418835 "Remove EventSender from HTMLLinkElement" was the first bad commit.

Hmm. Chrome waits for subresource image loading for Tampermonkey's popup anyway, even without r418835.
After r418835, there is delay after completion of all subresources and HTMLLinkElement::scheduleEvent() is called and before HTMLLinkElement::dispatchPendingEvent() is called (no subresource loading during that period).

Looks like scheduler issue?

1. An async task (that blocks Document onload) for HTMLLinkElement is posted to DOMManipulation TaskRunner.
2. Last subresource loading (amor.png) is completed.
   (Around this time, Document onload was called before r418835.)
3. Many other tasks are executed.
4. The async task posted in Step 1 is executed, and unblocks Document onload.
5. Document onload is called.

Only a few tasks including the async task in Step 1 experiences >500ms delay from postTask() to its execution.
If I use Timer's TaskRunner, the delay dissappeared (Test CL: https://codereview.chromium.org/2596343002/).

Does DOMManipulation TaskRunner have much lower priority than timerTaskRunner()?

And thus  Issue 657804  was not related (different cause).

Hmm, currently DOMManipulation and Timer task runners are exactly the same:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp?rcl=0&l=19

I think your patch makes a difference because in the former case the task runner is associated with a frame and in the latter case it isn't. Could the frame be hidden (offscreen), which would mean we are throttling its tasks?

Yes, the frame "chrome-extension://dhdgffkkebhmkfjojejmpbldmpobfkfo/action.html" is hidden -- it's the popup window that is hidden until document onload and then turns visible. In this case, the hidden frame's tasks are on the critical path of UI (the critical path to be visible).

Is the throttling logic affected by Document's state e.g. whether it is parsing, whether DOMContentLoaded is called?
(The throttled task by HTMLLinkElement is executed after Document's parsing is finished and before Document onload is called)

 Issue 676965  has been merged into this issue.

Still happening in 55.0.2883.87

A new regression appeared in 56.0.2914.0 that introduces the delay even for inlined <style> elements, here's the bisect:

430567 (good) - 430572 (bad)
https://chromium.googlesource.com/chromium/src/+log/3828f1d2..b8cb16f7?pretty=fuller
Obvious suspect is r430572 "Reland of Remove EventSender from HTMLStyleElement"

I've used the old version of Stylish extension for the bisect because the new one switched to <link> for CSS. The old one may be installed from its repo [1]. Note: to bisect reliably, in case the delay is not immediately obvious (usually on the first click), you need to click at least 10-20 times to mark the revision as 'good'.

P.S. I'd say the issue is a release blocker but I'm biased as I deem Chrome hardly usable without extensions.

[1]: https://github.com/stylish-userstyles/stylish-chrome/releases/tag/1.5.1

Another things is (but I'm not sure if this is caused by the delay) a second click at the extension icon sometimes closes the menu and sometimes re-opens it.

>a second click at the extension icon sometimes closes the menu and sometimes re-opens it.

It was introduced by r351582 (47.0.2524.0) in attempt to fix  issue 458655 . I've commented on that issue, let's wait until the next weekend for some dev to respond, then I'll submit a new bug report. Or you can do it.

Increasing the priority. Possible fixes are

1) Modify scheduling so that the tasks are not throttled even though the tasks are associated with a not-yet-visible frame.
   If this can be done, this would be a more general approach, but I'm not sure how to determine the "critical" tasks associated with a hidden frame.

2) Make the tasks not to be associated to frames to avoid to be throttling.
   This is quite ad-hoc (assuming the specific tasks in HTMLLinkElement and HTMLStyleElement are critical based on this issue, and thus potentially leaving other tasks in similar situations still being throttled) and might inhibit per-frame scheduling, but the implementation is quite easy and mergeable to beta.

Scheduling people, how do you think?

We chatted about this briefly. To summarize, I think we want to try and keep these tasks throttleable because they might end up causing performance problems in other cases. Instead, we thought about giving new frames a grace period while they can run unthrottled even if they're invisible. We've got similar logic for audio playback -related throttling and alexclarke@ is going to see if we can extend that to frame visibility too.

To expand on this slightly, the current logic initially applies 1s timer alignment for background webviews.  After 10s the budget based throttling kicks in.

I'm proposing that instead we do no throttling at all for 10s and then then budget based throttling. I think this will fix this problem (and maybe others) while still retaining the lion's share of the benefit of background throttling.

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

commit 368911e4215596d9e75fe64f54c9accd14b27ae5
Author: alexclarke <alexclarke@chromium.org>
Date: Wed Jan 11 20:45:05 2017

Don't throttle web views until they've been in the background for 10s

Previously background web views had timer alignment throttling applied immediately
and budget based throttling after 10s.  It looks like even the timer alignment throttling
on it's own is causing problems for new tabs where the visibility isn't initially known.
To fix this we're changing the policy so we don't do any throttling at all until the web
view has been in the background for 10s and then we turn budget based throttling &
alignment on.

Note this patch removes fast/dom/timer-throttling-hidden-page.html
because the 10s grace period means this test would be too slow.

Virtual time doesn't help because Date.now() (currently) isn't
overridden. There's no support for using a mock time source in
layout tests.  In any event there's plenty of C++ unit test coverage
for this so it's not too bad to remove the layout test.

BUG= 649942 

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

[modify] https://crrev.com/368911e4215596d9e75fe64f54c9accd14b27ae5/third_party/WebKit/LayoutTests/TestExpectations
[delete] https://crrev.com/b61b264655a67b06dd3937ba02e7b487cc35aacd/third_party/WebKit/LayoutTests/fast/dom/timer-throttling-hidden-page-expected.txt
[delete] https://crrev.com/b61b264655a67b06dd3937ba02e7b487cc35aacd/third_party/WebKit/LayoutTests/fast/dom/timer-throttling-hidden-page.html
[modify] https://crrev.com/368911e4215596d9e75fe64f54c9accd14b27ae5/third_party/WebKit/Source/platform/WebFrameScheduler.h
[modify] https://crrev.com/368911e4215596d9e75fe64f54c9accd14b27ae5/third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc
[modify] https://crrev.com/368911e4215596d9e75fe64f54c9accd14b27ae5/third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.h
[modify] https://crrev.com/368911e4215596d9e75fe64f54c9accd14b27ae5/third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl_unittest.cc
[modify] https://crrev.com/368911e4215596d9e75fe64f54c9accd14b27ae5/third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc
[modify] https://crrev.com/368911e4215596d9e75fe64f54c9accd14b27ae5/third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h
[modify] https://crrev.com/368911e4215596d9e75fe64f54c9accd14b27ae5/third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl_unittest.cc

This should be fixed now.

Yep, no delay in latest canary.
Will you backmerge it to 55 and 56?

I'd like to wait a few days to see if there's any perf regressions or other unexpected fallout from this.  If not then merging to M56 probably wouldn't be too bad, we would also need: 

c1fe2cb [scheduler] Delay budget-based throttling for 10s after backgrounded. by altimin · 5 weeks ago

OK I think the perf bots have cycled and this seems fine.  Lets request a merge for m56, I suspect the boat for m55 has sailed.

Your change meets the bar and is auto-approved for M56. Please go ahead and merge the CL to branch 2924 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Looks like c1fe2cb has already been merged in.  I'll try and merge this now.

OK that didn't apply cleanly, there's been more churn than I'd realized with third_party/WebKit/Source/platform/WebFrameScheduler.h

I'm not too keen on doing surgery in the branch so sorry I'm going to withdraw the merge request.

How painful is it to reconcile the changes?  This is a moderately severe bug, and waiting an additional 6 - 8 weeks is a little painful.

cc'ing M56 release owner if this has to be part of M56.

It's a tricky merge needing two or three other (big) patches to land first.  I'm  not keen on trying that in the branch.

Then, an alternative might be to land a quick workaround like https://codereview.chromium.org/2596343002/ (Comment #21) and merge it to M-56.
e.g.:
1. Land a workaround to ToT, let it stay on canary for 1 day.
2. Merge the workaround to M-56.
3. Revert the workaround on ToT (because it is not needed after alexclarke@'s CL (Comment #35))

The workaround might not be tested/baked sufficiently on canary because alexclarke@'s CL already fix the problem, though.

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

commit 812f90689e6637a5a4f13151dba661464716e29e
Author: hiroshige <hiroshige@chromium.org>
Date: Thu Jan 19 00:16:37 2017

Use Unthrottled instead of DOMManipulation in HTMLLinkElement/HTMLStyleElement

The async tasks for HTMLLinkElement/HTMLStyleElement::dispatchPendingEvent()
are on the critical paths to show popup windows for extensions, but
the DOMManipulation task runner is throttled because the tasks are executed
before the popup windows become visible, causing delay in showing popups.

The scheduler is fixed by [1], but [1] is not merged to M-56.
This CL makes a mergeable workaround by using TaskType::Unthrottled for these
tasks to fix the issue on M-56.

[1] https://codereview.chromium.org/2620743002

BUG= 649942 

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

[modify] https://crrev.com/812f90689e6637a5a4f13151dba661464716e29e/third_party/WebKit/Source/core/html/HTMLLinkElement.cpp
[modify] https://crrev.com/812f90689e6637a5a4f13151dba661464716e29e/third_party/WebKit/Source/core/html/HTMLStyleElement.cpp

Committed a workaround CL (Comment #47) and requesting merge it to M-56, speculatively, because M-57 branch cut is tomorrow.
If there're any issues (in the plan in Comment #46 or the CL itself) please comment and I'll handle them tomorrow.

This bug requires manual review: We are only 11 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Thank you hiroshige!

The workaround (Comment #47) is included in 57.0.2986.0 and thus included in the current canary/dev releases.
I'll wait for the milestone owner's decision for merging, while I'll revert the workaround on ToT not to include in M-57 release branch.

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

commit 1a7fcf622020ce0685a1a81a227d6b67650f8251
Author: hiroshige <hiroshige@chromium.org>
Date: Thu Jan 19 21:50:16 2017

Revert of Use Unthrottled instead of DOMManipulation in HTMLLinkElement/HTMLStyleElement (patchset #3 id:40001 of https://codereview.chromium.org/2596343002/ )

Reason for revert:
This CL is not needed for M-57 and thus
reverting before M-57 branch cut as planned.

Original issue's description:
> Use Unthrottled instead of DOMManipulation in HTMLLinkElement/HTMLStyleElement
>
> The async tasks for HTMLLinkElement/HTMLStyleElement::dispatchPendingEvent()
> are on the critical paths to show popup windows for extensions, but
> the DOMManipulation task runner is throttled because the tasks are executed
> before the popup windows become visible, causing delay in showing popups.
>
> The scheduler is fixed by [1], but [1] is not merged to M-56.
> This CL makes a mergeable workaround by using TaskType::Unthrottled for these
> tasks to fix the issue on M-56.
>
> [1] https://codereview.chromium.org/2620743002
>
> BUG= 649942 
>
> Review-Url: https://codereview.chromium.org/2596343002
> Cr-Commit-Position: refs/heads/master@{#444559}
> Committed: https://chromium.googlesource.com/chromium/src/+/812f90689e6637a5a4f13151dba661464716e29e

TBR=haraken@chromium.org,alexclarke@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 649942 

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

[modify] https://crrev.com/1a7fcf622020ce0685a1a81a227d6b67650f8251/third_party/WebKit/Source/core/html/HTMLLinkElement.cpp
[modify] https://crrev.com/1a7fcf622020ce0685a1a81a227d6b67650f8251/third_party/WebKit/Source/core/html/HTMLStyleElement.cpp

The change in #47 lgtm for merging into M56

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

commit b64349d476d54ffbc86da51fc51ec1d75413fd7c
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Thu Jan 19 23:19:52 2017

Use Unthrottled instead of DOMManipulation in HTMLLinkElement/HTMLStyleElement

The async tasks for HTMLLinkElement/HTMLStyleElement::dispatchPendingEvent()
are on the critical paths to show popup windows for extensions, but
the DOMManipulation task runner is throttled because the tasks are executed
before the popup windows become visible, causing delay in showing popups.

The scheduler is fixed by [1], but [1] is not merged to M-56.
This CL makes a mergeable workaround by using TaskType::Unthrottled for these
tasks to fix the issue on M-56.

[1] https://codereview.chromium.org/2620743002

BUG= 649942 

Review-Url: https://codereview.chromium.org/2596343002
Cr-Commit-Position: refs/heads/master@{#444559}
(cherry picked from commit 812f90689e6637a5a4f13151dba661464716e29e)

Review-Url: https://codereview.chromium.org/2641263002 .
Cr-Commit-Position: refs/branch-heads/2924@{#809}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/b64349d476d54ffbc86da51fc51ec1d75413fd7c/third_party/WebKit/Source/core/html/HTMLLinkElement.cpp
[modify] https://crrev.com/b64349d476d54ffbc86da51fc51ec1d75413fd7c/third_party/WebKit/Source/core/html/HTMLStyleElement.cpp

Summary:
M-55: Regressed, not fixed.
M-56: Merged [2] (#47, #54). [1] is not merged. Fix to be verified.
M-57: Fix [1] is committed (#35) and verified (#37). [2] is reverted (#52).

[1] https://codereview.chromium.org/2620743002
[2] https://codereview.chromium.org/2596343002

Thanks for the updates hiroshige@. It sounds like [1] affects  issue 675372  as well. Do you plan to merge it to M56? Do the already merged patches [2] affect  issue 675372 ?

[1] will not be merged to M-56: see alexclarke@'s comments (#42 and #45).

Probably [2] doesn't affect  Issue 675372 , because [2] only makes the specific tasks in HTMLLink/StyleElement (not related to IndexDB) unthrottled.