Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#393 closed defect (fixed)

Don't evaluate 404 error pages in hostenv_browser

Reported by: sylvain@… 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 sylvain@… 13 years ago.
Patch to check for status codes other than 200
gettext (670 bytes) - added by adam@… 13 years ago.
hostenv_browser.js: check http.status when !async
gettext-take2.patch (541 bytes) - added by anonymous 13 years ago.
gettext-take2.2.patch (541 bytes) - added by adam@… 13 years ago.
gettext-take3.patch (479 bytes) - added by adam@… 13 years ago.
use this one.
boolean.patch (794 bytes) - added by adam@… 13 years ago.
Fixed boolean logic in sync case

Download all attachments as: .zip

Change History (17)

Changed 13 years ago by sylvain@…

Attachment: hostenv_browser.js.patch added

Patch to check for status codes other than 200

Changed 13 years ago by adam@…

Attachment: gettext added

hostenv_browser.js: check http.status when !async

comment:1 Changed 13 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 13 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 13 years ago by adam@…

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 13 years ago by anonymous

Resolution: fixed
Status: newclosed

merged in [4248]

comment:5 Changed 13 years ago by anonymous

Milestone: 0.3.1

comment:6 Changed 13 years ago by adam@…

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 13 years ago by anonymous

Attachment: gettext-take2.patch added

Changed 13 years ago by adam@…

Attachment: gettext-take2.2.patch added

comment:7 Changed 13 years ago by adam@…

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

Changed 13 years ago by adam@…

Attachment: gettext-take3.patch added

use this one.

comment:8 Changed 13 years ago by alex

Resolution: fixed
Status: reopenedclosed

merged. Marking fixed.

comment:9 Changed 13 years ago by adam@…

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 13 years ago by adam@…

Attachment: boolean.patch added

Fixed boolean logic in sync case

comment:10 Changed 13 years ago by alex

Resolution: fixed
Status: reopenedclosed

merged in 4337

comment:11 Changed 12 years ago by (none)

Milestone: 0.3.1

Milestone 0.3.1 deleted

Note: See TracTickets for help on using tickets.