1707853 - Crash in [@ memcpy | nsJARInputStream::Read] - exception handling still not working

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect, P1)

Description

[Valentin Gosu [:valentin]]

It seems the CopyMemory hack in bug 1551562 doesn't work for EXCEPTION_IN_PAGE_ERROR for reasons I don't understand.
I did a test with std::copy and this one seems to work. I also managed to write a unit test that needs some manual interaction to work, but it's still better than nothing.

Comment 1

[Valentin Gosu [:valentin]]

Attachment: Bug 1707853 - Use std::copy instead of memcpy to ensure exception handling works r=#necko

Comment 2

[Valentin Gosu [:valentin]]

Attachment: Bug 1707853 - Add always-skipped unit test for documentation purposes r=#necko

This test is for documentation purposes only.
With some work we may be able to automate it, but sharing folders on windows seems to require elevated priviledges.

Depends on D113497

Comment 3

[Valentin Gosu [:valentin]]

https://treeherder.mozilla.org/jobs?repo=try&revision=d07ddb1cbbef7bd6e07abb8f4756414f3b86982f&selectedTaskRun=Dhgcrh3gScKWvmhap_Q2zQ.0

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ddb01633b9f
Use std::copy instead of memcpy to ensure exception handling works r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/2c5a50fb4d4e
Add always-skipped unit test for documentation purposes r=necko-reviewers,dragana

Comment 5

[Valentin Gosu [:valentin]]

[Tracking Requested - why for this release]:
Turns out the fix we did in bug 1551562 didn't fix the issue.
This one seems to work a little better - and has a test we can use to verify the fix.
Additionally I need to test this in a Win7 VM too, since most of the crashes happen on x86 Win7.

Comment 6

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/0ddb01633b9f
https://hg.mozilla.org/mozilla-central/rev/2c5a50fb4d4e

Comment 7

[Valentin Gosu [:valentin]]

Comment on attachment 9218617 [details]
Bug 1707853 - Use std::copy instead of memcpy to ensure exception handling works r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: Avoidable crashes in nsJARInputStream::Read will continue.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Since the crashes rarely/never happen on Nightly and this release cycle will be longer it would be better to verify it on Beta.
  The risk is low - we replace memcpy/CopyMemory with std::copy which should have similar performance but also the ability to throw/catch exceptions, which we need to prevent these crashes.
• String changes made/needed:

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9218617 [details]
Bug 1707853 - Use std::copy instead of memcpy to ensure exception handling works r=#necko

Approved for 89.0b6, thanks.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/079ea0a288c6
https://hg.mozilla.org/releases/mozilla-beta/rev/b165baa4c2f9