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

web: Properly panic when loading invalid SWF files (fix #14665) #14715

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

danielhjacobs
Copy link
Contributor

Note: The error when loading a page which is not an SWF is the same as the error when loading an SWF that does not exist for the purpose of code simplicity. This can be changed if desired.

@danielhjacobs
Copy link
Contributor Author

#6665 is related, and the main point listed in that issue will now cause a panic. However, it's still true that other asynchronous errors just log without being able to be handled without some custom JavaScript, so this doesn't fully close that issue.

@danielhjacobs danielhjacobs force-pushed the fix-extension-panic branch 2 times, most recently from e0f857c to 9182091 Compare January 13, 2024 05:46
@Lord-McSweeney
Copy link
Collaborator

I think having the message be the same for invalid SWF vs SWF download fail is confusing, but I'm not sure how differentiating would be implemented- adding a new UI function feels like overkill.

@danielhjacobs
Copy link
Contributor Author

danielhjacobs commented Jan 16, 2024

I think having the message be the same for invalid SWF vs SWF download fail is confusing, but I'm not sure how differentiating would be implemented- adding a new UI function feels like overkill.

Maybe this can take a boolean parameter invalidSwf: boolean. All other places calling it can switch to display_root_movie_download_failed_message(false)

protected displayRootMovieDownloadFailedMessage(): void {

Then before these lines:

} else if (
window.location.origin === this.swfUrl!.origin ||
// The extension's internal player page is not restricted by CORS
window.location.protocol.includes("extension")
) {

I can add this:

} else if (invalidSwf) {
    error.ruffleIndexError = PanicError.InvalidSwf;

And before this line:

I can add InvalidSwf,

And before this:

case PanicError.SwfFetchError:

I can add:

            case PanicError.InvalidSwf:
                errorBody = textAsParagraphs("error-invalid-swf");
                errorFooter = this.createErrorFooter([new PanicLinkInfo()]);
                break;

And then I can add a message for error-invalid-swf before this:

error-swf-fetch =
Ruffle failed to load the Flash SWF file.
The most likely reason is that the file no longer exists, so there is nothing for Ruffle to load.
Try contacting the website administrator for help.

@danielhjacobs
Copy link
Contributor Author

I thought of all that before making this PR, which is why my initial comment said I did it the way I did "for the purpose of code simplicity."

@danielhjacobs danielhjacobs force-pushed the fix-extension-panic branch 2 times, most recently from 784fd2d to c4b0f57 Compare January 16, 2024 15:22
@danielhjacobs
Copy link
Contributor Author

My new commit adds a message for invalid SWF files separate from the message for non-existent files and displays it in all appropriate situations (loading an existing non-SWF file via URL and loading an existing non-SWF file using the file selector).

@danielhjacobs danielhjacobs force-pushed the fix-extension-panic branch 3 times, most recently from d6f0840 to f9304a0 Compare January 17, 2024 16:09
@danielhjacobs danielhjacobs enabled auto-merge (rebase) January 17, 2024 22:20
@danielhjacobs danielhjacobs merged commit 8f2292c into ruffle-rs:master Jan 17, 2024
13 checks passed
@danielhjacobs danielhjacobs deleted the fix-extension-panic branch January 24, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants