New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 621926 link

Starred by 9 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Em-DOSBox fails in 32-bit Chrome 51, 52 and 53, but works in 50 and 64-bit

Reported by boris.gj...@gmail.com, Jun 21 2016

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

Steps to reproduce the problem:
1. Navigate to https://archive.org/details/msdos_Prince_of_Persia_1990 or https://archive.org/details/msdos_Wolfenstein_3D_1992 in 32-bit Chrome 51, 52 or 53.
2. Press round power button in middle of game screenshot to start game
3. Observe game behaviour

What is the expected behavior?
Game should start running and displaying its graphics.

What went wrong?
These and other DOS games from https://archive.org/details/softwarelibrary_msdos_games crash on startup. Prince of Persia gets a stack overflow and Wolfenstein 3D gets abnormal program termination. You see this text output to the DOS console window.

This happens in 32-bit Chrome 51.0.2704.84 from http://www.slimjetbrowser.com/chrome/win/chrome32_51.0.2704.84.exe and following current versions downloaded normally Google: 51.0.2704.103 m, 52.0.2743.41 beta-m and 53.0.2767.4 dev-m. They work in corresponding 64-bit versions, and Chrome 50.0.2661.75 and earlier versions.

Did this work before? Yes Works in http://www.slimjetbrowser.com/chrome/win/chrome32_50.0.2661.75.exe

Chrome version: 51.0.2704.84  Channel: n/a
OS Version: 10.0
Flash Version: Shockwave Flash 22.0 r0

These DOS games are using my Emscripten port of DOSBox, at https://github.com/dreamlayers/em-dosbox . I realize that saying "this big Emscripten compiled JavaScript application behaves wrong" probably isn't very helpful, but it's probably better to report the bug anyways. This certainly seems like a Chrome bug because it has worked in a bunch of Chrome versions up to and including 50 and still works in other browsers and 64-bit Chrome.
 

Comment 1 by tkent@chromium.org, Jun 21 2016

Labels: Needs-Bisect
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.
Cc: bsalo...@google.com rnimmagadda@chromium.org
Components: Internals>GPU
Labels: -Type-Bug -Pri-2 -Needs-Bisect M-52 Pri-1 Type-Bug-Regression
Owner: bsalomon@chromium.org
Status: Assigned (was: Unconfirmed)
======================================

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.
Project Member

Comment 4 by sheriffbot@chromium.org, Jun 22 2016

Labels: -M-52 M-53 MovedFrom-52
Moving this nonessential bug to the next milestone.

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

Comment 5 by rpop@chromium.org, Jun 22 2016

Labels: Stability-Crash
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.

Comment 6 by kbr@chromium.org, Jun 22 2016

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

Comment 7 by kbr@chromium.org, Jun 22 2016

Labels: Needs-Feedback

Comment 8 by kbr@chromium.org, Jun 22 2016

Cc: kbr@chromium.org
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)

Cc: bsalomon@chromium.org
Owner: epertoso@chromium.org
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.

Comment 11 by tkent@chromium.org, Jun 23 2016

Components: -Blink
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.
pi10frag.zip
1.0 MB Download
Cc: epertoso@chromium.org
Owner: bsalomon@chromium.org
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.
Owner: epertoso@chromium.org
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?
Labels: -Arch-x86_64 Arch-x86
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.
Labels: OS-Linux
Status: Fixed (was: Assigned)
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/
Labels: Merge-Request-52 Merge-Request-51

Comment 23 by dimu@google.com, Jun 30 2016

Labels: -Merge-Request-51 Merge-Review-51 Hotlist-Merge-Review
[Automated comment] Request affecting a post-stable build (M51), manual review required.

Comment 24 by dimu@google.com, Jun 30 2016

Labels: -Merge-Request-52 Merge-Approved-52 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M52 (branch: 2743)

Comment 25 by dimu@google.com, Jun 30 2016

Labels: -Merge-Request-51 Merge-Review-51 Hotlist-Merge-Review
[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.
Cc: ranjitkan@chromium.org
Error_Site.png
22.0 KB View Download
Project Member

Comment 29 by sheriffbot@chromium.org, Jul 4 2016

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
Project Member

Comment 31 by sheriffbot@chromium.org, Jul 7 2016

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
Labels: -Merge-Approved-52 -Merge-Review-51 backport-review merge-merged-52
epertoso@ does this also needs merging to 5.3?
Labels: -backport-review backport-done
epertoso@, could you please reply to 33? 
Labels: Merge-Request-53
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.
Labels: -Merge-Request-53 Merge-Approved-53
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.
Labels: -Merge-Approved-53
Thank you  epertoso@.
This is already merged to M53 branch at #39. So removing "Merge-Approved-53" label.
Labels: -Backport-Done NodeJS-Backport-Done

Comment 42 Deleted

Sign in to add a comment