Ensure IAccessible2 remains available in Firefox after restarting NVDA on Windows 10 Creaters update, register ISimpleDOM proxy to allow math in Google Chrome without Firefox installed, and Allow the user to get back a virtualBuffer when restarting NVDA with a Firefox document in focus by michaelDCurran · Pull Request #7535 · nvaccess/nvda

michaelDCurran · 2017-08-29T06:13:40Z

Link to issue number:

Fixes #7269
Fixes #7308
Partial fix for #5758
Fixes #7563

Summary of the issue:

Originally some users were complaining that when they restarted NVDA while Firefox was in focus, browse mode was no longer available. Alt+tabbing away and back again would also not correct the situation.
Specifically issue #5758 covers this original case. Technically, when NVDA restarts and Firefox is already in focus, There is no focus event for Firefox and therefore NVDA's in-proces injection code is not triggered for the Firefox process. Therefore we have no binding handle for creating a virtualBuffer. However, as VirtualBuffer.prepare has already set shouldPrepare to false by the time it reaches this error, it is impossible for the virtualBuffer to ever work again, even if a binding handle does become available by shifting focus away and back again.

There was a second issue (#7269) that started appearing in Windows insider builds approaching the Windows 10 Creaters Update. In this situation, restarting NVDA and then trying to use Firefox would result in NVDA not being able to ever QueryService to IAccessibleApplication to check the Gecko version number, and therefore NVDA would never provide browse mode. Restarting NVDA would not help. All that could be done was to close and re-open Firefox. There seemed to be some kind of COM proxy stub issue where if a request was made to marshal an interface that had just been deregistered with CoRevokeClassObject, then once that interface was again successfully registered, marshalling would be for ever still broken in the server process.

Finally, issue #7308 talks about not being able to read / navigate math content in Google chrome. The main reason for this is that Google Chrome does not register ISimpleDOM interfaces which are used between chrome and NVDA to fetch the math content. What confuses the situation is that if the user did have Firefox installed, math content would work in Google chrome as the interfaces were available via Firefox's registration.

Description of how this pull request fixes the issue:

#5758 is partially fixed by simply returning early in VirtaulBuffer.prepare if there is no binding handle yet. This means that on future focus changes, the VirtaulBuffer will again try to prepare. Once there is a binding handle, this will succeeed. In short, restarting NVDA while Firefox is in focus will still result in no browse mode at first, but the user can easily shift focus away and back again to get Browse mode.

#7269 and #7308 together are fixed by a substantial re-write to NVDAHelperRemote's COM proxy registration code.
Firstly, the code has been abstracted so that it is easy to register other COM proxies other than just IAccessible2. Now both IAccessible2 and ISimpleDOM are both built, and registered in all processes we inject into if COM is initialized. The abstraction also allows us to fetch all the interfaces from the dll directly, rather than having to hardcode them.
The COM proxy dlls now contain assembly manifests which aide in us getting COM to load the class objects itself, while manging the life time of the dll, saving us from having to free it on NVDA shutdown, which in 99% of cases never worked.
And, most importantly the specific problem introduced by Windows 10 creaters update where CoRevokeClassObject would break future registration was fixed by pointing the interfaces to the standard marshaller with CoRegisterPSClsid before their class object was revoked. The standard marshaller does not support the interfaces of course, but at least they still point to a valid class object. Microsoft really needs a CoUnregisterPSClsid.

Early iterations of this PR also showed up issue #7563, which this PR now also addresses by moving all NVDA dlls into a version-specific directory to stop accidental loading or reuse of a dll from a previous version which could cause a crash.

Testing performed:

Multiple restarts of NVDA both using control+alt+n, and the NVDA+q quit dialog (choosing restart), on both Windows 10 Creaters update and Windows 7 64 bit, while Mozilla Firefox is in focus.

Known issues with pull request:

Issue #5758 is not fully fixed as the user must still alt+tab away and back again to get Browse mode in Firefox when restarting NVDA with Firefox in focus.

Change log entry:

Bug fixes:

On windows 10 Creaters Update, NVDA can again access Firefox after a restart of NVDA. (Browse mode in Mozilla Gecko products no longer can be used after restarting NVDA due to IAccessible2 interface registration issues #7269)
When restarting NVDA with Mozilla Firefox in focus, browse mode will again be available, though you may need to alt+tab away and back again. (NVDA problem on start with Firefox in focus #5758)
It is now possible to access math content in Google Chrome on a system with out Mozilla Firefox installed. (Build and register ISimpleDOM proxy ourselves (AKA fix broken math in Firefox/Chrome on some systems) #7308)
The Operating System and other applications should be more stable directly after installing NVDA before rebooting, as compaired with installs of previous NVDA versions. (other apps may crash or cause errors when an NVDA installation is updated #7563)

…A on Windows 10 Creaters update. Also abstract code making it easy to register other COM proxy dlls.

…gle chrome.

…t's appModule's helperLocalBindingHandle is missing. There may not be any binding handle yet if NVDA was started and the document in question already had focus. All though the virtualBuffer will still not yet work, at least the user can now move focus away and back again and prepare will be tried again once the binding handle is there.

LeonarddeR · 2017-08-29T06:37:22Z

I assume this doesn't contribute anything to a fix of #7311?

michaelDCurran · 2017-08-29T07:23:12Z

This won't directly affect the problems in that issue. But all of these issues are similar in that COM marshalling is affected. We are planning to include some kind of utility in NVDA to fix issues such as #7311 and perhaps having it run in the NVDA installer also.

feerrenrut · 2017-08-29T13:21:07Z

+def clsidStringToCLSIDDefine(clsidString):
+	"""
+	Converts a CLSID string of the form "{abcdef12-abcd-abcd-abcd-abcdef123456}"
+	Into a c-style struct initializer for initializing a GUID (I.e. "{0xabcdef12,0xabcd,0xabcd,{0xab,0xab,0xab,0xab,0xab,0xab,0xab,0xab}}") 


If I understand this code right, I think the last part of this example result should be: {0xab, 0xcd, 0xab, 0xcd, 0xef, 0x12, 0x34, 0x56}

feerrenrut · 2017-08-29T13:28:05Z


 idlFile=env.Command("ia2.idl","#/miscDeps/include/ia2/ia2.idl",Copy("$TARGET","$SOURCE"))

+


extra new line

feerrenrut · 2017-08-29T13:30:42Z

-	LIBS=['rpcrt4','oleaut32','ole32'],
-	CPPDEFINES=[env['CPPDEFINES'],'WIN32','REGISTER_PROXY_DLL'],
-	LINKFLAGS=[env['LINKFLAGS'],'/export:DllGetClassObject,private','/export:DllCanUnloadNow,private'],
+	proxyClsid="{62d295fe-2062-4369-a010-4f59b5e32d5e}"


Where does this clsid come from?

feerrenrut · 2017-08-29T13:33:26Z

+/*
+This file is a part of the NVDA project.
+URL: http://www.nvda-project.org/
+Copyright 2006-2010 NVDA contributers.


The years listed are wrong. This also doesn't match the format we have on the wiki: https://github.com/nvaccess/nvda/wiki/Copyright-headers

feerrenrut · 2017-08-29T13:34:34Z

+
+typedef void(RPC_ENTRY *LPFNGETPROXYDLLINFO)(ProxyFileInfo***, CLSID**);
+
+const wchar_t* StringCLSID_StandardMarshaler=L"{00020424-0000-0000-C000-000000000046}";


perhaps you could add a comment to this line? Is there a reference for this value?

feerrenrut · 2017-08-29T14:47:44Z

+	}
+	// look up the GetProxyDllInfo function on the proxy dll 
+	LPFNGETPROXYDLLINFO Dll_GetProxyDllInfo=(LPFNGETPROXYDLLINFO)GetProcAddress(dllHandle,"GetProxyDllInfo");
+	if(Dll_GetProxyDllInfo==NULL) {


nullptr
And there are a few more, I wont comment on all of them.

feerrenrut · 2017-08-29T14:55:13Z

+	ACTCTX actCtx={0};
+	actCtx.cbSize=sizeof(actCtx);
+	actCtx.dwFlags=ACTCTX_FLAG_HMODULE_VALID|ACTCTX_FLAG_RESOURCE_NAME_VALID;
+	actCtx.lpResourceName=MAKEINTRESOURCE(2);


feerrenrut · 2017-08-29T15:08:33Z

+		reg->psClsidBackups.push_back({name,iid,clsidBackup});
+		LOG_DEBUG(L"Registered interface "<<name);
+		}
+		++tempInfoPtr;


Is there some docs you can point to on ProxyFileInfo? I'm a little nervous about this code. So pProxyInfo points to the start of a memory block containing ProxyFileInfo*. This memory block is null terminated. I cant find any documentation on Dll_GetProxyDllInfo How do you know the above is the case?

feerrenrut · 2017-08-29T15:12:44Z

+		LOG_DEBUG(L"Unregistered interface "<<(backup.name));
+	}
+	if((res=CoRevokeClassObject((DWORD)(reg->classObjectRegistrationCookie)))!=S_OK) {
+		LOG_ERROR(L"Error unregistering class object from "<<(reg->dllPath)<<L", code "<<res);


For this (and the other error case on line 156) should the function exit early, why is it ok to continue to delete reg and return true

feerrenrut · 2017-08-29T15:13:17Z

+/*
+This file is a part of the NVDA project.
+URL: http://www.nvda-project.org/
+Copyright 2006-2010 NVDA contributers.


See copyright headers https://github.com/nvaccess/nvda/wiki/Copyright-headers

… up as much as possible.

LeonarddeR · 2017-08-30T06:53:09Z

This seems to cause incidental errors, which I've seen in at least Thunderbird and Spotify when focussing the application

ERROR - RPC process 12692 (thunderbird.exe) (08:50:15.348):
Thread 12696, build\x86\remote\COMProxyRegistration.cpp, registerCOMProxy, 69:
GetProxyDllInfo function not found in IAccessible2Proxy.dll

ERROR - RPC process 12692 (thunderbird.exe) (08:50:15.361):
Thread 12696, build\x86\remote\ia2Support.cpp, installIA2Support, 59:
Error registering IAccessible2 proxy

michaelDCurran · 2017-08-30T06:56:13Z

Was this a source or binary copy? If binary, where exactly did you get it? That error is pretty much suggesting that a different copy of IAccessible2Proxy.dll was loaded instead of the one that actually came with that version of NVDA. Very very strange. At least, does the issue go away if you reboot?

LeonarddeR · 2017-08-30T07:27:47Z

This was a binary copy, Version next-14364,d19efb8d. Note that I hadn't yet rebooted, and I assume a reboot is likely to fix this. I'll report back if the problem still persists.

…XP." We are now planning to drop support for XP/Vista in PR #7546. This reverts commit 2aed84c.

…t Windows will always load the correct dll when hooking, rather than possibly falling back to a dll in a previous version that may currently still be loaded in a process.

michaelDCurran · 2017-09-04T07:34:47Z

This PR also fixes #7563, which was uncovered by other parts of this PR.

michaelDCurran added 3 commits August 29, 2017 13:59

Allow IAccessible2 to be used again in Firefox after a restart of NVD…

7c0e258

…A on Windows 10 Creaters update. Also abstract code making it easy to register other COM proxy dlls.

Build and register ISimpleDOM proxy ourselves allowing for math n Goo…

70a88f5

…gle chrome.

michaelDCurran requested a review from feerrenrut August 29, 2017 06:13

feerrenrut suggested changes Aug 29, 2017

View reviewed changes

michaelDCurran added 2 commits August 30, 2017 10:18

Address review actions.

730fd5f

NVDAHelperRemote's unregisterCOMProxy: don't return early so we clean…

fe71f7c

… up as much as possible.

feerrenrut approved these changes Aug 30, 2017

View reviewed changes

michaelDCurran added a commit that referenced this pull request Aug 30, 2017

Incubates #7535

d19efb8

nvaccessAuto added the incubating label Aug 30, 2017

nvaccessAuto assigned michaelDCurran Aug 30, 2017

LeonarddeR mentioned this pull request Aug 30, 2017

Next Snapshots won't install on Windows XP any more #7544

Closed

Remove need for CoGetApartmentType as it is not supported on XP.

2aed84c

michaelDCurran added a commit that referenced this pull request Aug 30, 2017

Incubates #7535

cee6c42

michaelDCurran mentioned this pull request Aug 31, 2017

Limit NVDA to running only on at least Windows 7 SP1 or Windows Server 2008 R2 SP1 #7546

Merged

Revert "Remove need for CoGetApartmentType as it is not supported on …

2e6ad21

…XP." We are now planning to drop support for XP/Vista in PR #7546. This reverts commit 2aed84c.

LeonarddeR mentioned this pull request Aug 31, 2017

Firefox does not gain focus when started from Windows 7 with latest snapshots #4296

Closed

michaelDCurran added 4 commits September 2, 2017 12:19

Make sure to unhook the inproc winEvent on normal termination!

b8a9177

Merge branch 'unhookInprocWinEvent' into i7308

dbb9ab2

Merge branch 'master' into i7308

b42ffa6

Place all NVDA dlls in a version-specific lib directory to ensure tha…

952dfe3

…t Windows will always load the correct dll when hooking, rather than possibly falling back to a dll in a previous version that may currently still be loaded in a process.

michaelDCurran mentioned this pull request Sep 4, 2017

other apps may crash or cause errors when an NVDA installation is updated #7563

Closed

michaelDCurran requested a review from feerrenrut September 4, 2017 07:42

feerrenrut approved these changes Sep 4, 2017

View reviewed changes

michaelDCurran added a commit that referenced this pull request Sep 4, 2017

Incubates #7535

0531065

michaelDCurran merged commit bfee0f5 into master Sep 19, 2017

nvaccessAuto removed the incubating label Sep 19, 2017

nvaccessAuto added this to the 2017.4 milestone Sep 19, 2017

LeonarddeR mentioned this pull request Sep 20, 2017

Undesirable side effect of versioned lib path: scons source required after every commit or branch change #7614

Closed

gregjozk mentioned this pull request Oct 3, 2017

toggle button does not expand itself #7640

Closed

This was referenced Oct 4, 2017

When NVDA is running from source, dlls are no longer stored in a version-specific directory #7644

Merged

Installer: try to clean up old library files at the end of installation with out affecting the rest of a successful install #7649

Merged

Neurrone mentioned this pull request Jan 12, 2018

NVDA is frequently unable to gain focus when alt-tabbing back into VS Code microsoft/vscode#28316

Closed

Adriani90 mentioned this pull request Jul 28, 2018

NVDA problem on start with Firefox in focus #5758

Closed


		idlFile=env.Command("ia2.idl","#/miscDeps/include/ia2/ia2.idl",Copy("$TARGET","$SOURCE"))


		typedef void(RPC_ENTRY LPFNGETPROXYDLLINFO)(ProxyFileInfo, CLSID);

		const wchar_t* StringCLSID_StandardMarshaler=L"{00020424-0000-0000-C000-000000000046}";

Uh oh!

Conversation

michaelDCurran commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

LeonarddeR commented Aug 29, 2017

Uh oh!

michaelDCurran commented Aug 29, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeonarddeR commented Aug 30, 2017

Uh oh!

michaelDCurran commented Aug 30, 2017 via email

Uh oh!

LeonarddeR commented Aug 30, 2017 via email

Uh oh!

michaelDCurran commented Sep 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

michaelDCurran commented Aug 29, 2017 •

edited

Loading