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

Fix panic on "unreachable code" that is reachable #267

Merged
merged 1 commit into from May 26, 2019

Conversation

@dralley
Copy link
Contributor

dralley commented May 26, 2019

@dralley
Copy link
Contributor Author

dralley commented May 26, 2019

According to the discussion on the linked issue, cancellation became supported in this issue servo/servo#22367

This is my first servo PR :)

@Manishearth
Copy link
Member

Manishearth commented May 26, 2019

This isn't the correct fix, these items are not supposed to appear in the timeline, they get dealt with during insertion. The correct fix is to move this return to the outer if https://github.com/dralley/media/blob/d06aac618ed1884eb4a5d0b64a86bd371a60017f/audio/param.rs#L210

@Manishearth
Copy link
Member

Manishearth commented May 26, 2019

(we haven't implemented the difference between Cancel and CancelHold correctly, but that's for another time.)

@dralley dralley force-pushed the dralley:master branch from d06aac6 to 90320ad May 26, 2019
@dralley
Copy link
Contributor Author

dralley commented May 26, 2019

@Manishearth I'm still getting the issue - is this patch what you had in mind or did I move it to the wrong place?

@dralley dralley force-pushed the dralley:master branch from 90320ad to 50ab227 May 26, 2019
@dralley
Copy link
Contributor Author

dralley commented May 26, 2019

[dalley@localhost servo]$ ./mach run https://midi.city
%c * Tone.js r7 * 
background: #000; color: #fff
internal error: entered unreachable code: CancelScheduledValues/SetValue should never appear in the timeline (thread AudioRenderThread, at /home/dalley/Devel/servo-dev/media/audio/param.rs:333)
stack backtrace:
}
dbg!(":?", &event);

This comment has been minimized.

@dralley

dralley May 26, 2019

Author Contributor

Prints nothing

@Manishearth
Copy link
Member

Manishearth commented May 26, 2019

@dralley Are you sure it's been recompiled using your local branch? I don't see how this is possible at all.

@dralley
Copy link
Contributor Author

dralley commented May 26, 2019

@Manishearth yup I'm sure

[dalley@localhost servo]$ ./mach build --dev
   Compiling servo-media-audio v0.1.0 (/home/dalley/Devel/servo-dev/media/audio)
   Compiling servo-media v0.1.0 (https://github.com/servo/media#ecd5a7c6)

With a debug at the very top, it does print this

[dalley@localhost servo]$ ./mach run https://midi.city
[/home/dalley/Devel/servo-dev/media/audio/param.rs:182] ":?" = ":?"
[/home/dalley/Devel/servo-dev/media/audio/param.rs:182] &event = SetValue(
    0.0,
)
%c * Tone.js r7 * 
background: #000; color: #fff
[/home/dalley/Devel/servo-dev/media/audio/param.rs:182] ":?" = ":?"
[/home/dalley/Devel/servo-dev/media/audio/param.rs:182] &event = CancelScheduledValues(
    Tick(
        128,
    ),
)
internal error: entered unreachable code: CancelScheduledValues/SetValue should never appear in the timeline (thread AudioRenderThread, at /home/dalley/Devel/servo-dev/media/audio/param.rs:335)
@dralley dralley force-pushed the dralley:master branch from 50ab227 to 36a0764 May 26, 2019
@Manishearth
Copy link
Member

Manishearth commented May 26, 2019

Can you paste the full backtrace?

@Manishearth
Copy link
Member

Manishearth commented May 26, 2019

Oh. Right.

unreachable!("CancelScheduledValues/SetValue should never appear in the timeline")

That should return the inner time. My bad.

@Manishearth
Copy link
Member

Manishearth commented May 26, 2019

But the others should still panic

@dralley dralley force-pushed the dralley:master branch from 36a0764 to eafa387 May 26, 2019
@dralley
Copy link
Contributor Author

dralley commented May 26, 2019

How about now?

@Manishearth
Copy link
Member

Manishearth commented May 26, 2019

@bors-servo r+

Feel free to also add a test for this

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2019

📌 Commit eafa387 has been approved by Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2019

Testing commit eafa387 with merge 783d5ba...

bors-servo added a commit that referenced this pull request May 26, 2019
Fix panic on "unreachable code" that is reachable

Fixes servo/servo#22580
@dralley
Copy link
Contributor Author

dralley commented May 26, 2019

@Manishearth I have no idea how to add a test for this unfortunately, first Servo PR. I'd be willing to do so but I'd need some guidance

@Manishearth
Copy link
Member

Manishearth commented May 26, 2019

No biggie. If you want to try, you need to add another example under examples/ (make sure you add it to Cargo.toml. Perhaps copy one of the params examples and make it mess with cancel messages.

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2019

☀️ Test successful - checks-travis
Approved by: Manishearth
Pushing 783d5ba to master...

@bors-servo bors-servo merged commit eafa387 into servo:master May 26, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.