975452 - Implement metro widgets for input thread separation

Status: RESOLVED WONTFIX

(Core Graveyard :: Widget: WinRT, defect, P1)

Description

[Jim Mathies [:jimm]]

Attachment: wip

TODO:
  - implement required nsIWidget method stubs
  - add override stubs we use in MetroWidget
  - figure out if we can use MetroWidget for MetroWidgetInput without
    breaking non thread sep functionality. We might just go ahead and
    copy code over to a new MetroWidgetInput class. Probably simpler.
  - deal with tricky sFrameworkView calls on the gecko side in MetroApp
  - implement communication between MetroWidgetGecko and MetroWidgetInput
  - get MetroWidgetInput talking to the apzc
  - get MetroInput talking to MetroWidgetInput
  - test MetroApp browser shutdown sequence

Comment 1

[Jim Mathies [:jimm]]

Tim is this something you could pick up and run with, or would you prefer I take this a bit further?

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Yeah I'd be super happy to sink my teeth into this!

Comment 3

[Jim Mathies [:jimm]]

One helpful tip - when you build this up you can find the ipdl generated headers here -

obj\ipc\ipdl\_ipdlheaders\mozilla\widget\winrt

you'll need the patches in bug 974901 & bug 974989. if those don't apply cleanly ping me on irc.

Comment 4

[Jim Mathies [:jimm]]

One other tip - we had talked about how input events between the two widgets would transfer from one side to the other as gecko events. Thankfully we already have gecko event serializers for these which saves us a ton of work - 

http://mxr.mozilla.org/mozilla-central/source/widget/nsGUIEventIPC.h

Comment 5

[Jim Mathies [:jimm]]

Attachment: wip

Updated to get things building.

I think we should limit the work in this bug to getting the plumbing fleshed out and the browser painting. The more complex interfaces between the widgets (ime, input) and things like apzc support can get fleshed out in other bugs.

Comment 6

[Jim Mathies [:jimm]]

I've worked around the original issue with the constructor, but I've come up on something else that's more serious. We attempt to connect when the first widget gets created, but this happens before nsAppShell::Run is called and as such the gecko thread isn't in a chromium message loop yet. So when the input thread attempts to connect, there's no loop set up on the other side. Not sure yet how to fix this.

Comment 7

[Jim Mathies [:jimm]]

Attachment: additional changes, on top of wip

Comment 8

[Jim Mathies [:jimm]]

Attachment: combined wip

This combines the winrt code from bug 974989 plus all the changes here.

Comment 9

[Jim Mathies [:jimm]]

I was thinking about the idle service problem, I think the best solution in cases like this would be to check to see if we're running on the main thread before accessing the main thread resource. If we are, make the call, if we aren't bounce the call to the main thread using an nsRunnable class.

We might find a use for a common set of nsRunnables included via a common header, or maybe this problem isn't that common.

Comment 10

[Jim Mathies [:jimm]]

Attachment: part 1 - metro app thread separation

Breaking these up so they are more manageable.

Comment 11

[Jim Mathies [:jimm]]

Attachment: widget wip

Comment 12

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #9)
> I was thinking about the idle service problem, I think the best solution in
> cases like this would be to check to see if we're running on the main thread
> before accessing the main thread resource. If we are, make the call, if we
> aren't bounce the call to the main thread using an nsRunnable class.
> 
> We might find a use for a common set of nsRunnables included via a common
> header, or maybe this problem isn't that common.

Note this won't solve the case where we need a return result, and as it turns out I found another of these in nsBaseWidget::BaseCreate, which initializes a new nsDeviceContext which creates a nsIScreenManager.

We'll have to take each of these on individually to get a feel for how wide spread the problem is.

Comment 13

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Part 1 - metro app thread separation - v2

Updated to apply cleanly over 170933:4cfb6c61b137

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Part 2 - widget - v2 (WIP)

Updated to apply cleanly over the part 1 patch

Comment 15

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Misc WIP patch v1

This patch contains miscellaneous fixes that I've been using to get metroFx running.

1) It enables the metroFx debug delay
2) It comments out most of MetroUtils::ScaleFactor and just returns 1.0 - WinRT was throwing a RoOriginateError when we tried to do "dispInfoStatics->GetForCurrentView(&dispInfo)"
3) It makes MetroWidget::AnswerLateCreate create and initialize an nsDeviceContext, then pass that in to the call to |Create|. I think we'll actually want to keep this change.
4) It expands the macros that were used to implement AddRef, QI, and Release for nsScreenManagerWin. I'm not sure why, but when the macros were being used I was seeing an exception thrown in AddRef and the debugger was reporting that I was in nsSimpleArrayEnumerator::AddRef.

Next to tackle: A RoOriginateError is being thrown from FrameworkView::ActivateView when called from MetroApp::ActivateBaseView. This looks like it might be happening on the wrong thread (FrameworkView probably shouldn't be accessed from the gecko thread)

Comment 16

[Jim Mathies [:jimm]]

> 4) It expands the macros that were used to implement AddRef, QI, and Release
> for nsScreenManagerWin. I'm not sure why, but when the macros were being
> used I was seeing an exception thrown in AddRef and the debugger was
> reporting that I was in nsSimpleArrayEnumerator::AddRef.

xpcom interfaces that aren't thread safe (most of them) assert if the thread accessing them isn't the thread that creates the object - 
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#54
I'd bet this is the source of the assertions.

Comment 17

[Jim Mathies [:jimm]]

Yeah I bet the assert is in the component manager somewhere when the input thread tries to create the screen manager via do_GetService.

This is the heart of the problem we'll have to solve. MetroWidget(input) can't access non-thread safe xpcom interfaces directly. We'll have to find a way to bypass these types of calls. I think you can get away with just stubbing stuff like screen manager out in base widget. MetroWidget(input) doesn't need it, so there's no point in initializing or accessing it.

MetroWidgetGecko on the other hand will have to call BaseCreate and create a valid screen manager. We have to look at these interfaces and figure out what they do - if they query for system information that can happen on the gecko thread, great. If they query for information only the input thread can access, we'll need our own version of these objects that intr calls over to a corresponding input thread class.

Both the MetroApp and MetroWidget protocols can create additional protocols (objects) when we need them.

Comment 18

[Jim Mathies [:jimm]]

If you go the root you were thinking of taking, landing this all at once, MetroWidget doesn't need to inherit from base widget or implement nsIWidget, which solves the screen manager problem and other potential xpcom issues in that class.

Comment 19

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I don't expect to work on metroFx in the near future, though it would be fun to try to use the lessons/approach here for desktopFx.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

We never shipped the metro support, closing!