Opened 15 years ago

Closed 14 years ago

Last modified 13 years ago

#393 closed defect (fixed)

Don't evaluate 404 error pages in hostenv_browser

Reported by: [email protected] Owned by: anonymous
Priority: high Milestone:
Component: General Version: 0.3
Keywords: Cc:
Blocked By: Blocking:

Description

dojo.hostenv.getText doen't check the http response code of synchronous loads, and therefore returns back the text of any 404 error page back to loadURI. This produces syntax errors.

Attachments (6)

hostenv_browser.js.patch (490 bytes) - added by [email protected] 15 years ago.
Patch to check for status codes other than 200
gettext (670 bytes) - added by [email protected] 14 years ago.
hostenv_browser.js: check http.status when !async
gettext-take2.patch (541 bytes) - added by anonymous 14 years ago.
gettext-take2.2.patch (541 bytes) - added by [email protected] 14 years ago.
gettext-take3.patch (479 bytes) - added by [email protected] 14 years ago.
use this one.
boolean.patch (794 bytes) - added by [email protected] 14 years ago.
Fixed boolean logic in sync case

Download all attachments as: .zip

Change History (17)

Changed 15 years ago by [email protected]

Attachment: hostenv_browser.js.patch added

Patch to check for status codes other than 200

Changed 14 years ago by [email protected]

Attachment: gettext added

hostenv_browser.js: check http.status when !async

comment:1 Changed 14 years ago by anonymous

syntax errors are usually caught (but still expensive and unnecessary) See tests/i18n/test_strings.html running over http for examples where it's more obvious.

I created a patch before I found this bug (see gettext, attached) It's like Sylvain's, but a bit more current.

comment:2 Changed 14 years ago by alex

hey adam,

so the patch looks straightforward, but I'm not sure why throwing an exception here is a good thing. Won't this screw up the "walk up the directory tree" algorithm?

Regards

comment:3 Changed 14 years ago by [email protected]

the exception throw inside the try block is caught and only re-thrown if fail_ok is false, same code path as if http.send() threw an exception, but that doesn't always happen.

this is basically equivalent to Sylvain's patch, just a tiny bit shorter.

comment:4 Changed 14 years ago by anonymous

Resolution: fixed
Status: newclosed

merged in [4248]

comment:5 Changed 14 years ago by anonymous

Milestone: 0.3.1

comment:6 Changed 14 years ago by [email protected]

Resolution: fixed
Status: closedreopened

whoops. thought I tested on file:/// but apparently did not. new patch to check for http.status being set to something non-zero

Changed 14 years ago by anonymous

Attachment: gettext-take2.patch added

Changed 14 years ago by [email protected]

Attachment: gettext-take2.2.patch added

comment:7 Changed 14 years ago by [email protected]

you can see I'm not doing that well tonight. it's been a long day. try gettext-take3.patch

Changed 14 years ago by [email protected]

Attachment: gettext-take3.patch added

use this one.

comment:8 Changed 14 years ago by alex

Resolution: fixed
Status: reopenedclosed

merged. Marking fixed.

comment:9 Changed 14 years ago by [email protected]

Resolution: fixed
Status: closedreopened
Version: 0.20.3

Who thought a couple lines of code could cause so much trouble? :-| I was also thinking of doing more thorough error checking on 2xx in my patch but was disappointed that I couldn't do integer division in JS.

Perhaps hiding the error and the boolean negation with a private function would be clearer? I'm also not certain if 300 http error codes can get redirected by XHR and end up being ok? At least we're no worse off than we were before.

Changed 14 years ago by [email protected]

Attachment: boolean.patch added

Fixed boolean logic in sync case

comment:10 Changed 14 years ago by alex

Resolution: fixed
Status: reopenedclosed

merged in 4337

comment:11 Changed 13 years ago by (none)

Milestone: 0.3.1

Milestone 0.3.1 deleted

Note: See TracTickets for help on using tickets.