http://defacto2.net/file/detail/b32bc68?dosmachine=vga&dosspeed=auto causes an "Aw, Snap!" tab crash in Chrome. It also uses Em-DOSBox. After the URL loads, you need to click where it says "Click or tap here to start", and then wait. It will say "Time warping to DOS", and after that, when the DOS prompt appears the tab crashes. For me it crashes 51.0.2704.84 32-bit but works in 50.0.2661.75 32-bit and 51.0.2704.84 64-bit. All are in 64-bit Windows 10 build 14367. 

That means it behaves like the other problems, and the same bug may be causing the crash and emulator misbehaviour. JavaScript should not crash browser processes. It might even have security implications, if specially crafted JavaScript could use the bug to access things JavaScript shouldn't be able to access.

======================================

Good Build:

51.0.2696.0      Base Position: 384437


Bad Build:

51.0.2697.0      Base Position: 384752

======================================

Able to repro this issue on Windows 7 for the Google Chrome Stable Version - 51.0.2704.103

This is a regression issue broken in M51, below mentioned is the bisect info:

CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/1bd259470b934427f2ba2ced349e6b00007ad3fe..4b96b8dba3653f9152b01206974f710fb78131c0

https://chromium.googlesource.com/skia.git/+log/38d68bc31d69..31dcc2a16f1b

Suspecting Commit: 342bfc25de5b0452b1551bf9db4bf45eac7718b2

Review URL: https://codereview.chromium.org/1835283002

@bsalomon: Could you please look into the issue, and if it has nothing to do with your changes and if possible please do assign it to the concerned owner.

Thank you.

Note: Issue is not observed on MAC (10.11.5) & Linux (Ubuntu Trusty [14.04]) OS.

Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Are there any entries in chrome://crashes that correspond to the times that the emulator failed to load? If so, please provide crash IDs here as well.

Yes, please provide crash IDs. At this point not many of the Chrome developers are running the 32-bit browser.

Here are 3 crashes that just happened on http://defacto2.net/file/detail/b32bc68?dosmachine=vga&dosspeed=auto after clicking "Click or tap here to start" and waiting a bit for unreal.exe to start running in the emulator. That causes a crash every single time for me. This is 32-bit Chrome 51.0.2704.103 m with a clean profile, and only the extensions Google bundles.

Crash ID 8a4196e600000000 (8106e889-e7db-4ff2-854a-27164b969c0b)
Crash ID 56d196e600000000 (6bfbd26d-0421-464e-9bb2-6125829226f1)
Crash ID fe0896e600000000 (b24676b2-8c59-49f4-8a00-929842a17dda)

I've manually bisected this down to this v8 roll:

ed221ce Update V8 to version 5.1.247. by v8-autoroll · 3 months ago

and specifically this V8 change in that roll:

https://chromium.googlesource.com/v8/v8/+/3dd3beb06676fe8030d1fef9086cbe63e4a4376d
[ia32] Byte and word memory operands in ia32 cmp/test.

The smallest misbehaving DOS program I could find was the pi_10.com QEMU test.
The pi_10.com source is in: http://web.archive.org/web/20160305205129/http://boo.net/~jasonp/bf3pi.zip
Found it via this page: http://web.archive.org/web/20160305205129/http://boo.net/~jasonp/pipage.html

Seems like "adc dx, bp" at offset 0x24 is causing the problem. I stripped pi_10.com down to an even smaller .com file which still triggers the problem, and am attaching it here. Open pi10frag.html. Since it needs other files there, either do this via a web server like python -m SimpleHTTPServer or start Chrome with the --allow-file-access-from-files command line option.

When the program works, it ought to output 0 to the DOS window and finish, returning to the C:\> prompt. In Chrome 51.0.2704.103 m it also outputs a single character and finishes, but outputs various characters. Reloading the page often but not always changes the character.

I wonder why the character is changing? Is it reading from wrong memory locations? Is it reading outside of what JavaScript should be able to read from?

The CPU interpreter is a loop which calls various functions based on the opcode. This should have narrowed it down to a fairly small number of functions. It is possible to build Em-DOSBox with different Emscripten parameters which result in more readable JavaScript code. Let me know if assistance is needed with that.

Deleted: pi10frag.zip 1.0 MB

pi10frag.zip 1.0 MB Download

I realized that the Skia read pixels call is grabbing a large chunk of stack space (64K) and the crash from the reports is in _chkstk. Tomorrow morning I'll try removing that allocation and and see if it crashes. It could be that the V8 change is not directly related but just nudged us over the edge of available stack space.

Removing that stack allocation doesn't allow the Prince of Persia game to load successfully. I'm doing a debug build and will run in the debugger to see what I can find.

I ran a debug build and didn't get a crash or notice any different logging with or without the v8 change. Kicking this back over to v8. I'll still remove that stack allocation from Skia but I don't think it is related to the failure to load the DosBox games at archive.org.

I'm skeptical about the bisect which found https://chromium.googlesource.com/skia/+/342bfc25de5b0452b1551bf9db4bf45eac7718b2 "Simplify GrDrawBatch uploads and token uage."
Graphical stuff could only be causing games to fail to work like that if it was causing memory corruption for JavaScript. Also, a crash involving that might be due to memory corruption caused by JavaScript.

If memory corruption is occurring, changes elsewhere in Chrome might cause a crash to not happen, causing misleading bisect results. Bisecting based on games working or not working would be more certain.

For all I know, maybe there are two issues here: the crash on that one game and other games not working. I merely suspect that it's the same issue because in both cases it works in 32-bit version 50 and in 64-bit version 51.

The JavaScript change found by bisecting seems far more probable: https://chromium.googlesource.com/v8/v8/+/3dd3beb06676fe8030d1fef9086cbe63e4a4376d "[ia32] Byte and word memory operands in ia32 cmp/test."
If wrong x86 instructions are being generated and executed, that could explain everything.

I'm on it. I can observe the 'aw snap' at http://defacto2.net/file/detail/b32bc68?dosmachine=vga&dosspeed=auto in 32bit Chromium on Linux too.

Enrico, did you find anything more here?

Yes, I horribly swapped two opcodes when adding cmpw to assembler-ia32.cc. This was causing the order of the operands in a cmpw to be reversed (hence recursions that don't terminate and such). 

There's a fix (https://codereview.chromium.org/2103713003/). I'm trying to figure out why the v8_mac_rel_ng_triggered is failing, so that I can submit it.

http://crrev.com/2103713003 is now in. I was able to play through the first level of Prince of Persia, Wolfenstein 3D starts too and so does http://defacto2.net/file/detail/b32bc68?dosmachine=vga&dosspeed=auto.

When it's verified I can merge the change in stable and dev too.

Thank you! Everything seems fine in the latest 32-bit binaries from http://chromium.woolyss.com/

[Automated comment] Request affecting a post-stable build (M51), manual review required.

Your change meets the bar and is auto-approved for M52 (branch: 2743)

[Automated comment] Request affecting a post-stable build (M51), manual review required.

Rechecked this on chrome version 53.0.2785.0 (32 Bit) Version on Windows 7 OS. Still able to reproduce it. Attached screenshot for the same.

@epertoso: Request you to please take a look into it.

Seems that the V8 update containing my fix was reverted in 53.0.2785.0: https://chromium.googlesource.com/chromium/src/+log/53.0.2784.0..53.0.2785.0?pretty=fuller&n=10000

Here: https://chromium.googlesource.com/chromium/src/+/0d4110d82c8b9c3d743aa6d14e63ca5aaec0c241

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This was merged in 5.2 with https://codereview.chromium.org/2125873002

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

epertoso@ does this also needs merging to 5.3?

epertoso@, could you please reply to 33? 

I though this was done, but actually it seems it didn't make it to 5.3.

This should be a safe merge: CL https://codereview.chromium.org/2103713003/ doesn't depend on other CLs and the files it touches aren't updated that often.

Approving merge to M53 branch 2785 based on comment #37. Please merge latest by Monday, 09/05 5:00 PM PT. Thank you.


If M54 merge is needed, please request a merge. 

Merged in http://crrev.com/2311793002

M54 does not need a cherry-pick.

Thank you  epertoso@.
This is already merged to M53 branch at #39. So removing "Merge-Approved-53" label.