Re: Moving alert() away from nested message loop

[Ojan Vafai]

+platform-architecture-dev 

FWIW, I agree that this is orthogonal to OldSpice.

On Wed, Apr 13, 2016 at 9:05 PM Jochen Eisinger <eisi...@google.com> wrote:

Explanation is here: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/render_thread_impl.cc&l=978

Might no longer be true though... +piman

On Thu, Apr 14, 2016, 5:14 AM Avi Drissman <a...@google.com> wrote:

I don't actually know. :(

I got involved in alert dialogs when I implemented them for the Mac, and later when I worked in that area for content. I'm pretty up on the UI half, but know nothing about how the mechanisms behind them work. If it is indeed a legacy requirement from showModalDialog, and if nowadays we can actually downgrade them to a less messy implementation, I'm all for it. That's probably independent of any work that I do on OldSpice, yes? Given that such a change wouldn't be visible to the web, I would say to go for it if you can.

BTW, OldSpice is going to take a while to get all the UI in line, get agreement on implementations, and get coordination across browser vendors. I would highly recommend that you do not wait for it if possible :)

Avi

On Wed, Apr 13, 2016 at 11:08 PM, Alexandre Elias <ael...@google.com> wrote:

Hi Avi,

Following up on https://codereview.chromium.org/1865143003/ -- can you answer my question about why alert() uses a nested message loop?  My mental model is that alert() needs to block on a single result, the Yes/No boolean answer from the browser-side dialog box.  A full nested message loop that processes every message is overkill for this and introduces a requirement on a lot of other subsystems to support possible reentrancy and reordering.  If we simply need to wait on a single message type, why not use an ordinary sync IPC for alert?  (Or, if we need to also process one or two other types of messages, why not specifically filter those in the nested message loop and repost out every unknown message to the outer loop?)

I'm hoping the answer is that it's implemented this way purely as a legacy of showModalDialog() and that there's no remaining reason why we can't switch to sync IPC.  If that's the case, changwan@ has volunteered to switch alert() to it as it will unblock his project and he has a decent understanding of the problem space.  It seems that fixing the nested message loop is unfortunately *not* a part of the current OldSpice plan, but maybe we can add it in?

If it's not so easy, please explain why!  cc'ing also danakj@ who is working on nested-message-loop primitives for other use cases in the browser like resize -- if we are forced to use nested message loops after all, we should at least have a single shared implementation that fits all the uses of it in Chrome.

--
Alex

[Elliott Sprehn]

I don't think we need the full nested message loop for alert(), why does resize need one?

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CANMdWTs6qNSUeOBOMnp1%3D30jvu5HyKRHY3Y3fNmxoEj-gT0gfw%40mail.gmail.com.

[Ojan Vafai]

readding... +platform-architecture-dev 

On Wed, Apr 13, 2016 at 10:40 PM John Abd-El-Malek <jabde...@google.com> wrote:

This comment is outdated and should be removed or updated after the removal of NPAPI.

There are still a few places where IPC::SyncMessage::EnableMessagePumping() is called, which is what triggers this code path. JS dialogs are one of them. Even ignoring plugins, this behavior was originally added to maintain compatibility with IE and Firefox. They implemented JS dialogs by calling win32's MessageBox on the same thread where the html engine was running. Internally that runs a nested message loop. So we wanted animations to continue running to match their behavior. A simple example: https://jsfiddle.net/sfc75sg6/1/

I tested this now and while animations continue in IE and Edge, this doesn't work anymore in Safari and Firefox. My hunch is we don't care to maintain this behavior anymore, in which case we can simplify this a lot, but I defer to folks more knowledgeable about the web.

On Wed, Apr 13, 2016 at 9:15 PM, Avi Drissman <a...@google.com> wrote:

Are you talking about the comment?

  // This could cause

  // a complete hang of Chrome if a windowed plugin is trying to communicate

  // with the renderer thread

We have no windowed plugins any more (goodbye, NPAPI!) so does this still apply?

+jam

[Alexandre Elias]

Thanks, it sounds like the animation behavior demonstrated by https://jsfiddle.net/sfc75sg6/1/ is the main reason we still have this.  Readding [+platform-architecture-dev]

I agree it doesn't seem worth the burden unless there is a concrete use case for that behavior.  Perhaps we could land a revertable change to match Safari and Firefox on this, send a Web-facing Change PSA to blink-dev@ and see if it sticks to stable channel, and if so make the change permanent.

--
Alex

[Ojan Vafai]

I made a slightly broader test case: https://jsfiddle.net/sfc75sg6/5/.

When a dialog is open:

• Safari and Firefox both don't run marquee or main thread animations.
• They both run hardware accelerated CSS animations.
  
• Chrome and Safari pause animated gifs.
• Firefox keeps running animated gifs.

I also made a version that has audio and video and noone seems to pause those when a dialog is open (although Firefox went a bit crazy here). Doing a sync IPC only blocks the main thread right? In that case, I think we'd match the Safari behavior.

I expect we'd all be OK with shipping that, but it is a breaking change, so it'll need to go through the usual web platform change process. Alex/Changwan, I can guide you through that. It shouldn't be much overhead in this case.

Arch leads, that sound good to you?

[Elliott Sprehn]

Sgtm, simpler is better.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CANMdWTsyyv1pTTbZrg7KMnfduH5sjzw6N8DJvb9HiwY%2Bn6LwXQ%40mail.gmail.com.

[Antoine Labour]

On Wed, Apr 13, 2016 at 9:05 PM, Jochen Eisinger <eisi...@google.com> wrote:

Explanation is here: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/render_thread_impl.cc&l=978

Might no longer be true though... +piman

I took a quick look when removing the NPAPI code, because I thought the whole IPC feature (run a nested message loop while sending) might be scrappable. It's still used in some places, IIRC among others for PPAPI Flash context menus, though I can't remember the reason (Brett might know, I think he did it, points finger). So I didn't try to remove. It'd be nice to fix, though.

Or at the very least, like Alex suggests, a limited-purpose "message loop" that only reenters things that we want, like resize in the browser.

To answer Elliott's question, the reason we run a limited purpose "message loop" during resize is this:

- the renderer usually takes longer to respond to resize messages than resize events come in / the browser compositor can consume new frames

- if we run the renderer fully asynchronously like we usually do, it looks super janky with tons of gutters, because by the time the renderer responds with a frame of the requested size, the browser has moved on and resized again

- so instead of drawing all bad frames at 60 fps, it's a lot better to draw all good frames at 15-20 fps, by waiting (with a timeout) for the renderer to give us a frame after a resize.

- to make sure we don't let the browser move forward with more resizes while we're waiting, at least on Windows and Mac, we need to block the resize event handler. But it's only very deep in the stack - essentially because UI layout is recursive - that we know that we need to resize the renderer.

- so we need the equivalent of a sync message (with timeout) to the renderer for the resize, and for the compositor to draw a frame after that

- however, for the renderer and the compositor to make progress, we need to handle *some* reentrant messages (but not all, and remember we're deep in the stack where arbitrary reentrancy is a bad idea), as well as some tasks posted by the compositor.

- so we run this limited purpose "message loop" (which is not really a message loop, just a ThreadTaskRunner), that picks up relevant messages on the IO thread, and only dispatches those as well as the tasks posted by the compositor.

All that to say, a similar technique may be applicable to several of those things that need to pump messages while waiting for a reply, if they can't be converted to a straight sync message (which is better/simpler). I think that was Alex's suggestion if I understood correctly. 

Antoine

[Elliott Sprehn]

This nested message loop you're talking about is in the browser process though right? Could the browser process instead mark the renderer as needing a resize, unwind the layout stack, notice there's dirty web contents then install a message loop filter? In blink we wouldn't try to process something like this sync under the layout system, we'd use dirty bits or store the state and then process it when coming out. It allows better asserts and lifecycle control.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAMeTaZcJvdOL%3DxR3Q_KLB5Pj%2BiHAJD7hJ%2BB7FVStrQy2LCXc5w%40mail.gmail.com.

[Antoine Labour]

On Thu, Apr 14, 2016 at 8:35 PM, Elliott Sprehn <esp...@chromium.org> wrote:

This nested message loop you're talking about is in the browser process though right?

Yes

 

Could the browser process instead mark the renderer as needing a resize, unwind the layout stack, notice there's dirty web contents then install a message loop filter?

I'm not sure what you mean by message loop filter. The point is either way we can't return from the (e.g.) WM_SIZE handler, so we'd have to still run the nested task runner, at the top of the stack. It can be indeed safer, but the current solution actually works extremely well, and is well-contained. I'm not sure there's benefits to doing the large refactoring you're suggesting - which would introduce layering violations because the UI layers responsible for the message handler don't know anything about renderers.

 

In blink we wouldn't try to process something like this sync under the layout system, we'd use dirty bits or store the state and then process it when coming out. It allows better asserts and lifecycle control.

That's fantastic.

I imagine though, you can't really do that for the cases we were talking about (e.g. window.alert()) where I assume you can't unwind the JS stack including the related blink bits.

[Ojan Vafai]

aelias and I chatted, next steps are:

1. Make a proposal to share with other browser vendors at https://github.com/WICG/interventions/issues. 

2. If this is in a spec somewhere, make a patch to the spec that we attach to that issue.

3. Send an intent to implement and ship. This doesn't need to block on #1/#2 above completing, but it should block on the discussions having been started.

4. Ship.

Domenic or Tab, would you be willing to help with #1 and #2 here? The specific thing is that we want to change the behavior of web pages when a modal dialog is open to match Safari. Namely, accelerated CSS animations continue, but main thread rendering and animated gifs are paused (currently in Chrome the main thread keeps pumping). It's not clear to me what spec would even cover this. Is it the HTML spec? I know the HTML spec has the main thread rendering pipeline.

[Domenic Denicola]

On Fri, Apr 15, 2016 at 7:21 PM Ojan Vafai <oj...@google.com> wrote:

aelias and I chatted, next steps are:

1. Make a proposal to share with other browser vendors at https://github.com/WICG/interventions/issues. 

2. If this is in a spec somewhere, make a patch to the spec that we attach to that issue.

3. Send an intent to implement and ship. This doesn't need to block on #1/#2 above completing, but it should block on the discussions having been started.

4. Ship.

Domenic or Tab, would you be willing to help with #1 and #2 here? The specific thing is that we want to change the behavior of web pages when a modal dialog is open to match Safari. Namely, accelerated CSS animations continue, but main thread rendering and animated gifs are paused (currently in Chrome the main thread keeps pumping). It's not clear to me what spec would even cover this. Is it the HTML spec? I know the HTML spec has the main thread rendering pipeline.

Sure, happy to help. This will require some research, definitely, as I'm not sure how to disentangle things.

The current spec situation, as far as I know, is that the spec says:

> Optionally, pause while waiting for the user to acknowledge the message.

which pauses the entire message loop so that no tasks or rendering or script can continue to run. That seems like it would preclude CSS animations from running to me, since they are part of the update the rendering idea currently, due to the simplistic model of how rendering works in the spec.
 

[Ojan Vafai]

On Fri, Apr 15, 2016 at 7:52 PM Domenic Denicola <dom...@google.com> wrote:

On Fri, Apr 15, 2016 at 7:21 PM Ojan Vafai <oj...@google.com> wrote:

aelias and I chatted, next steps are:

1. Make a proposal to share with other browser vendors at https://github.com/WICG/interventions/issues. 

2. If this is in a spec somewhere, make a patch to the spec that we attach to that issue.

3. Send an intent to implement and ship. This doesn't need to block on #1/#2 above completing, but it should block on the discussions having been started.

4. Ship.

Domenic or Tab, would you be willing to help with #1 and #2 here? The specific thing is that we want to change the behavior of web pages when a modal dialog is open to match Safari. Namely, accelerated CSS animations continue, but main thread rendering and animated gifs are paused (currently in Chrome the main thread keeps pumping). It's not clear to me what spec would even cover this. Is it the HTML spec? I know the HTML spec has the main thread rendering pipeline.

Sure, happy to help. This will require some research, definitely, as I'm not sure how to disentangle things.

The current spec situation, as far as I know, is that the spec says:

> Optionally, pause while waiting for the user to acknowledge the message.

which pauses the entire message loop so that no tasks or rendering or script can continue to run. That seems like it would preclude CSS animations from running to me, since they are part of the update the rendering idea currently, due to the simplistic model of how rendering works in the spec.

Ah. I'm much less fussed about allowing the things we happen to let through already (accelerated animations) than I am about disallowing the things we let through today that we shouldn't (main thread animations). If I read the spec correctly, this change actually makes us more spec compatible and we could just ship it today. So, I think we could just do 3 and 4 above to ship this.

Although, I do find the following from the spec super confusing: " User agents should remain responsive to user input while paused, however, albeit in a reduced capacity since the event loop will not be doing anything." If the event loop is paused, then we can't remain responsive to user input.

It seems like we'd need to have the spec understand that there's another event loop for stuff that happens on the compositor thread to spec this properly? +Ian Kilpatrick +Ian Vollick since this is effectively about scheduling the rendering pipeline.

In either case, Alex, I think you can move forward here with just sending the intent to implement and ship that clarifies what web-exposed behavior is changing since this makes us more spec-compatible. 

[Domenic Denicola]

On Mon, Apr 18, 2016 at 2:28 PM Ojan Vafai <oj...@google.com> wrote:

On Fri, Apr 15, 2016 at 7:52 PM Domenic Denicola <dom...@google.com> wrote:

On Fri, Apr 15, 2016 at 7:21 PM Ojan Vafai <oj...@google.com> wrote:

aelias and I chatted, next steps are:

1. Make a proposal to share with other browser vendors at https://github.com/WICG/interventions/issues. 

2. If this is in a spec somewhere, make a patch to the spec that we attach to that issue.

3. Send an intent to implement and ship. This doesn't need to block on #1/#2 above completing, but it should block on the discussions having been started.

4. Ship.

Domenic or Tab, would you be willing to help with #1 and #2 here? The specific thing is that we want to change the behavior of web pages when a modal dialog is open to match Safari. Namely, accelerated CSS animations continue, but main thread rendering and animated gifs are paused (currently in Chrome the main thread keeps pumping). It's not clear to me what spec would even cover this. Is it the HTML spec? I know the HTML spec has the main thread rendering pipeline.

Sure, happy to help. This will require some research, definitely, as I'm not sure how to disentangle things.

The current spec situation, as far as I know, is that the spec says:

> Optionally, pause while waiting for the user to acknowledge the message.

which pauses the entire message loop so that no tasks or rendering or script can continue to run. That seems like it would preclude CSS animations from running to me, since they are part of the update the rendering idea currently, due to the simplistic model of how rendering works in the spec.

Ah. I'm much less fussed about allowing the things we happen to let through already (accelerated animations) than I am about disallowing the things we let through today that we shouldn't (main thread animations). If I read the spec correctly, this change actually makes us more spec compatible and we could just ship it today. So, I think we could just do 3 and 4 above to ship this.

Although, I do find the following from the spec super confusing: " User agents should remain responsive to user input while paused, however, albeit in a reduced capacity since the event loop will not be doing anything." If the event loop is paused, then we can't remain responsive to user input.

Heh, that is a strange turn of phrase. I bet this is trying to say "The back button should still work." I.e. the "reduced capacity" is the ability for the page to respond to any input, while the browser chrome still should be able to.

 

It seems like we'd need to have the spec understand that there's another event loop for stuff that happens on the compositor thread to spec this properly? +Ian Kilpatrick +Ian Vollick since this is effectively about scheduling the rendering pipeline.

Yeah, that sounds about right. I'm happy to put this in the bucket of "Houdini people will take over speccing rendering one day and HTML will just defer to them", as was vaguely discussed in the past.

[Alexandre Elias]

On Mon, Apr 18, 2016 at 11:27 AM, Ojan Vafai <oj...@google.com> wrote:

On Fri, Apr 15, 2016 at 7:52 PM Domenic Denicola <dom...@google.com> wrote:

On Fri, Apr 15, 2016 at 7:21 PM Ojan Vafai <oj...@google.com> wrote:

aelias and I chatted, next steps are:

1. Make a proposal to share with other browser vendors at https://github.com/WICG/interventions/issues. 

2. If this is in a spec somewhere, make a patch to the spec that we attach to that issue.

3. Send an intent to implement and ship. This doesn't need to block on #1/#2 above completing, but it should block on the discussions having been started.

4. Ship.

Domenic or Tab, would you be willing to help with #1 and #2 here? The specific thing is that we want to change the behavior of web pages when a modal dialog is open to match Safari. Namely, accelerated CSS animations continue, but main thread rendering and animated gifs are paused (currently in Chrome the main thread keeps pumping). It's not clear to me what spec would even cover this. Is it the HTML spec? I know the HTML spec has the main thread rendering pipeline.

Sure, happy to help. This will require some research, definitely, as I'm not sure how to disentangle things.

The current spec situation, as far as I know, is that the spec says:

> Optionally, pause while waiting for the user to acknowledge the message.

which pauses the entire message loop so that no tasks or rendering or script can continue to run. That seems like it would preclude CSS animations from running to me, since they are part of the update the rendering idea currently, due to the simplistic model of how rendering works in the spec.

Ah. I'm much less fussed about allowing the things we happen to let through already (accelerated animations) than I am about disallowing the things we let through today that we shouldn't (main thread animations). If I read the spec correctly, this change actually makes us more spec compatible and we could just ship it today. So, I think we could just do 3 and 4 above to ship this.

Although, I do find the following from the spec super confusing: " User agents should remain responsive to user input while paused, however, albeit in a reduced capacity since the event loop will not be doing anything." If the event loop is paused, then we can't remain responsive to user input.

It seems like we'd need to have the spec understand that there's another event loop for stuff that happens on the compositor thread to spec this properly? +Ian Kilpatrick +Ian Vollick since this is effectively about scheduling the rendering pipeline.

In either case, Alex, I think you can move forward here with just sending the intent to implement and ship that clarifies what web-exposed behavior is changing since this makes us more spec-compatible. 

Sounds good, here is the draft intent (open for comments in case I wrote something inaccurate), I'll send it out tomorrow: https://docs.google.com/document/d/12bBKWupJy9JFoDIA3hfDavkz9wvEqGcPl-6Em6pmYDQ/edit#