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

panic when trying to load url which does not begin with http(s) #6184

Open
zmike opened this issue May 25, 2015 · 6 comments
Open

panic when trying to load url which does not begin with http(s) #6184

zmike opened this issue May 25, 2015 · 6 comments
Labels

Comments

@zmike
Copy link
Contributor

@zmike zmike commented May 25, 2015

    fn on_load_url_window_event(&mut self, url_string: String) {
        debug!("osmain: loading URL `{}`", url_string);
        self.got_load_complete_message = false;
        let root_pipeline_id = match self.scene.root {
            Some(ref layer) => layer.get_pipeline_id(),
            None => panic!("Compositor: Received WindowEvent::LoadUrl without initialized compositor \
                           layers"),
        };
    vvvvv this line causes a panic
        let url = Url::parse(&url_string).unwrap();
        self.window.set_page_url(url.clone());

        let msg = ConstellationMsg::LoadUrl(root_pipeline_id, LoadData::new(url));
        let ConstellationChan(ref chan) = self.constellation_chan;
        chan.send(msg).unwrap()
    }
@jdm
Copy link
Member

@jdm jdm commented May 26, 2015

Isn't that usually the responsibility of the frontend to fix up?

@zmike
Copy link
Contributor Author

@zmike zmike commented May 26, 2015

Adding it, possibly. But the engine still shouldn't crash if it can't parse a url.

@brunoabinader
Copy link
Contributor

@brunoabinader brunoabinader commented May 26, 2015

Will get on that, if no one else already started doing so :-)

@Manishearth
Copy link
Member

@Manishearth Manishearth commented May 26, 2015

We could also be trying to load a file called mozilla.org.

@zmike
Copy link
Contributor Author

@zmike zmike commented May 26, 2015

According to resources linked by @jdm, this is impossible since it would be a relative path file open which is not something that web browsers are capable of.

Mainly the fix I'm looking for here is to check the result and then send an error back to the window. I've added the required error functionality in #6175 which is in the reviewing queue.

@nox nox added the I-crash label May 29, 2015
@zmike
Copy link
Contributor Author

@zmike zmike commented May 29, 2015

#6175 has merged, so sending the appropriate error (ADDRESS_INVALID I think) should basically be a one-liner.

@jdm jdm added I-panic and removed I-crash labels Jul 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.