Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge all vbufBackend dlls into nvdaHelperRemote.dll #8866

Merged
merged 5 commits into from Oct 30, 2018

Conversation

Projects
None yet
5 participants
@michaelDCurran
Copy link
Contributor

michaelDCurran commented Oct 21, 2018

Link to issue number:

Closes #7641

Summary of the issue:

After updating NVDA while Firefox or Chrome web browsers are open, NVDA's virtualBuffers in these browsers fail to reflect further updates to web pages, and in some cases the browser crashes.
This is caused by:

  1. Although the old NVDA has quit, and the new NVDA has started, Windows may be slow at unloading the old NVDA's nvdaHelperRemote.dll. The old dll is fully terminated, but still resides in memory for a while.
  2. Focus moves into the web browser, and Windows loads the new NVDA's nvdaHelperRemote.dll into the browser, via an in-process winEvent.
  3. The new NVDA detects a document in a web browser and prepairs to create a virtualBuffer, by making an rpc call to the new nvdaHelperRemote.dll, requesting it to load the correct vbufBackend dll and create the virtualBuffer.
  4. The vbufBackend dll is linked against nvdaHelperRemote, so Windows tries to resolve this linkage by newly loading, or reusing an existing loaded nvdaHelperRemote.dll.
  5. As Two different nvdaHelperRemote dlls are loaded, Windows seems to choose the oldest one.
  6. In this case, Windows chooses the old NVDA's nvdaHelperRemote.dll.
  7. From now on, the virtualBuffer makes calls to the wrong nvdaHelperRemote, which is in fact terminated, and therefore it cannot make use of features such as nvdaHelperRemote's winEvent registrations etc. In deed, it is likely that one of the calls may completely crash the app, if for instance Windows does eventually unload the old NVDAHelperRemote.dll.

Description of how this pull request fixes the issue:

This PR removes the possibility of loading the wrong nvdaHelperRemote.dll by moving all the virtualBuffer code into nvdaHelperRemote.dll itself, which removes the need for the vbufBackend dlls all together.
The vbufBackend dls were origianlly separate as there was some hope that perhaps one day we might allow custom backends. However, in truth this was never fully possible due to other limitations in NVDA. Another reason was that vbufBackend dlls may link against some specific heavy-weight libraries, which in many cases would not be required in all apps except the ones that actually required that specifica virtualBuffer. In truth though, none of the vbufBackend dlls link against anything else other than what nvdaHelperRemote already links against.
A part from solving the dynamic changes dying, and the crashes, a further advantage is that NvDA's size shrinks significantly as we were carrying around about 12 vbufBackend dlls, all containing their own static copy of the CRT. Now, the virtualBuffers all share the one crt in nvdaHelperRemote.dll.

Testing performed:

Ran NVDA. Opened documents that require virtualBuffers in Firefox, Chrome, Internet Explorer, and webKit (iTunes). AdobeAcrobat, AdobeFlash and LotusNotes are yet to be tested. However, the only way these could fail is if the name if the virtualBuffer has been typed incorrectly in the source code. The rest of the code stays the same.

Known issues with pull request:

None.

Change log entry:

Bug fixes:
After updating NVDA, NvDA no longer fails to notice updates to document in Firefox and these browsers should no longer crash after the NVDA update. (#7641)

@michaelDCurran michaelDCurran requested a review from feerrenrut Oct 21, 2018

return NULL;
}

auto i=VBufBackendFactoryMap.find(backendName);

This comment has been minimized.

@feerrenrut

feerrenrut Oct 24, 2018

Contributor

If backendName is nullptr then I don't think it will convert to a wstring

Suggested change
auto i=VBufBackendFactoryMap.find(backendName);
if(!backendName){
LOG_ERROR(L"BackendName is nullptr");
return nullptr;
}
auto i=VBufBackendFactoryMap.find(backendName);
backend->initialize();
// Stop nvdaHelperRemote from being unloaded while a backend exists.
HINSTANCE tempHandle=nullptr;
if(!GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS,(LPCTSTR)dllHandle,&tempHandle)) {

This comment has been minimized.

@feerrenrut

feerrenrut Oct 24, 2018

Contributor

Could we replace the c-style cast in here (LPCTSTR)dllHandle

}
return TRUE;
}
UINT WM_HTML_GETOBJECT=RegisterWindowMessage(L"WM_HTML_GETOBJECT");

This comment has been minimized.

@feerrenrut

feerrenrut Oct 24, 2018

Contributor

This potentially subtly changes the order/timing of when this happens. This might not be a problem, but it's worth being aware of. If possible, I would prefer to see an explicit initialisation function.

@michaelDCurran

This comment has been minimized.

Copy link
Contributor Author

michaelDCurran commented Oct 24, 2018

@feerrenrut I have addressed your review comments.

@michaelDCurran michaelDCurran merged commit 24f5123 into master Oct 30, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2018.4 milestone Oct 30, 2018

michaelDCurran added a commit that referenced this pull request Oct 31, 2018

michaelDCurran added a commit that referenced this pull request Oct 31, 2018

Revert "Ensure that labels explicitly set on divs and spans are repor…
…ted in braille and speech" (#8899)

* Revert "Revert "Don't announce 'selected' when the focus moves in Google sheets if the focused cell is the only cell selected (#8879)" (#8893)"

This reverts commit b4e9e83.

* Revert "Don't announce 'selected' when the focus moves in Google sheets if the focused cell is the only cell selected (#8879)"

This reverts commit 51f5b2f.

* Revert "Merge all vbufBackend dlls into nvdaHelperRemote.dll (#8866)"

This reverts commit 24f5123.

* Revert "Fix handling of table cells without a containing table in browse mode. (#8887)"

This reverts commit 5fe34c5.

* Revert "Ensure that  labels explicitly set on divs and spans are reported in braille and speech (#8886)"

This reverts commit fd24d81.
@JulienCochuyt

This comment has been minimized.

Copy link
Contributor

JulienCochuyt commented Nov 28, 2018

Apologies for disturbing if this is irrelevant: Beware two branches were created with similar names but different case: mergedVbufBackendLibs (lower b) and mergedVBufBackendLibs (upper B), the former being ahead of the latter by 5 commits.

@leonardder

This comment has been minimized.

Copy link
Collaborator

leonardder commented Dec 11, 2018

We can remove the mergedVBufBackendLibs branch, but I'm not going to do that without @michaelDCurran's consent.

@michaelDCurran

This comment has been minimized.

Copy link
Contributor Author

michaelDCurran commented Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.