- Dillo-dev - lists.mailman3.com

CAPI ignores download requests when not in cache or handled by the downloads DPI.

by corvid＠lavabit.com

Jeremy wrote: > I wonder what is the real benefit of a separate downloads DPI. Is it > actually useful to have one downloads window for all Dillo processes? > The downloads DPI is rather different from all the rest: it is the > only one that links to FLTK and it doesn't send HTML back to Dillo. > Do we really benefit from putting downloading in a separate process? I think I would prefer to save things using dillo directly rather than having dillo get something, figure out that it can't display it, abort the connection, have this other process start over again in another window... (I tend to use wget by hand in order to avoid all of that.)

Jan. 1, 2013

Dpi_connect_socket retry

by jcid＠dillo.org

On Sun, Dec 30, 2012 at 07:11:28PM -0300, Jorge Arellano Cid wrote: > Hi, > > On Sat, Dec 29, 2012 at 05:44:22AM +0400, p37sitdu at lavabit.com wrote: > > On Thu, Dec 27, 2012 at 04:37:36PM -0300, Jorge Arellano Cid wrote: > > > As from the patch you sent (same as before), and the > > > explanation you give (ignoring my explanation), I guess you're > > > making conclusions out of reading the code in > > > Dpi_connect_socket() *only*. > > > > I made conclusions from reading Dpi_connect_socket() only, because in > > the case of error in Dpi_blocking_write file descriptor is left open > > and not passed anywhere else. If it was passed somewhere else, I would > > read the code there and check if file descriptor was closed there. > > > > > Have you checked what you state as fact? (i.e. that the FD is > > > leaked and not closed by the CCC). FWIW, I did check before > > > writing my previous answer. > > > > Patch that I did for checking is attached. It replaces > > Dpi_blocking_write call with -1 to simulate fault in > > Dpi_blocking_write for Dpi_connect_socket(). > > > > Introducing the error in Dpi_blocking_write for all functions doesn't > > work, in this case Dpi_connect_socket is not called at all (because > > Dpi_start_dpid fails). > > > > With this patch applied, I recompiled dillo and pressed "bookmarks" > > button several times to trigger dpi code. Each time I pressed > > "bookmarks" button, new descriptor was leaked: > > > > % ls /proc/$(pidof dillo)/fd > > 0 1 2 3 4 5 > > % ls /proc/$(pidof dillo)/fd > > 0 1 2 3 4 5 6 > > % ls /proc/$(pidof dillo)/fd > > 0 1 2 3 4 5 6 > > % ls /proc/$(pidof dillo)/fd > > 0 1 2 3 4 5 6 7 > > % ls /proc/$(pidof dillo)/fd > > 0 1 2 3 4 5 6 7 8 > > > > Then I applied the patch I sent before (with hg qpush) on top of the > > fault patch and rebuilt dillo: "make clean && make". I ran dillo again > > with ./src/dillo and did the same test with bookmarks button: > > > > % ls /proc/$(pidof dillo)/fd > > 0 1 2 3 4 5 > > % ls /proc/$(pidof dillo)/fd > > 0 1 2 3 4 5 > > % ls /proc/$(pidof dillo)/fd > > 0 1 2 3 4 5 > > > diff -r 8e10859b2aab src/IO/dpi.c > > --- a/src/IO/dpi.c > > +++ b/src/IO/dpi.c > > @@ -641,7 +641,7 @@ > > /* send authentication Key (the server closes sock_fd on error) */ > > } else if (!(cmd = a_Dpip_build_cmd("cmd=%s msg=%s", "auth", SharedKey))) { > > MSG_ERR("[Dpi_connect_socket] Can't make auth message.\n"); > > - } else if (Dpi_blocking_write(sock_fd, cmd, strlen(cmd)) == -1) { > > + } else if (-1 == -1) { > > MSG_ERR("[Dpi_connect_socket] Can't send auth message.\n"); > > } else { > > ret = sock_fd; > > > Ah, OK, now I see what you mean. > > There's a semantic ambiguity, the comment: > > /* send authentication Key (the server closes sock_fd on error) */ > > means: "if there's an error with the authentication" (I know because > I wrote the comment). > > (and as the patch removed this comment) I made the same test but with: > > - } else if (!(cmd = a_Dpip_build_cmd("cmd=%s msg=%s", "auth", SharedKey))) { > + } else if (!(cmd = a_Dpip_build_cmd("cmd=%s msg=%s","auth",SharedKey+1))) { > > and the CCC closes the file descriptor as expected. > > > The error case you noticed exists, and although highly unlikely > to happen, requires some thought. As from the test above, the > socket is connected, then nothing is written to simulate an > error, but the OS never sees an error, as it doesn't exist, and > doesn't notify either end (IOW, it leaves the socket passively > expecting for data to come). > > Now, the canonical way to handle the whole process should have > been to build the CCC upon connect(), then try to authenticate, > and let the CCC handle any errors that may come, but this is > probably too much code for little gain, so it was all coded in > Dpi_connect_socket(). > > The problem with closing FDs outside the CCC is that it can > introduce race conditions, but in this case the CCC is partially > built, so your patch suggestion may be OK. > > I'll have to give it some more thought and tests... OK, just committed a slightly modified and bigger patch that also corrects the wrong error error message given (CCC side). Please check how it works, and send feedback. -- Cheers Jorge.-

Jan. 1, 2013

CAPI ignores download requests when not in cache or handled by the downloads DPI.

by onepoint＠starurchin.org

p37sitdu at lavabit.com wrote: > On Sun, Dec 30, 2012 at 05:23:18PM +0000, Jeremy Henty wrote: > > > > I wondered why "Save link as" silently failed on some of my local > > web pages if I had not already visited the link. It turns out > > they were "file:///" links and "Save link as" sends a download > > request which the CAPI silently ignores if the URL is not cached > > and is not handled by the downloads DPI. > > [...] > > [...] > Instead, Dillo could download files and pass them to downloads DPI. > Downloads DPI then shows progress and saves files. As I understand, > the idea is to have one window for all downloads across many copies > of Dillo where progress can be shown and requests to cancel them can > be passed to the right copy of Dillo. I wonder what is the real benefit of a separate downloads DPI. Is it actually useful to have one downloads window for all Dillo processes? The downloads DPI is rather different from all the rest: it is the only one that links to FLTK and it doesn't send HTML back to Dillo. Do we really benefit from putting downloading in a separate process? Regards, Jeremy Henty

Jan. 1, 2013

Render bug

by Hole.destructor＠gmx.de

On Mon, Dec 31, 2012 at 07:52:11PM +0000, corvid wrote: > What if we do something like the attached? We might be able to get away > with a smaller fix, but this seemed sensible conceptually. Thanks, your patch fixes this issue (and I don't see any side effects). Best, Alex

Dec. 31, 2012

Render bug

by corvid＠lavabit.com

Alexander wrote: > Dear Dillo developers, > > sorry for not creating a proper bug report, but the "How to reproduce" > message field is too small for the example file. :) > > The issue > ========= > > After closing the "Save As" dialog, the background color of the HTML > page is not redrawn correctly (leaving white rectangles instead of the > background color). > > How to reproduce > ================ > > 1. open the appended html file (note the grey background) > 2. click on "file" link > 3. move the "Save As" window over the grey background > 4. close the "Save As" window --> parts of the background are drawn > white, instead of grey What if we do something like the attached? We might be able to get away with a smaller fix, but this seemed sensible conceptually.

Dec. 31, 2012

Render bug

by Hole.destructor＠gmx.de

Dear Dillo developers, sorry for not creating a proper bug report, but the "How to reproduce" message field is too small for the example file. :) The issue ========= After closing the "Save As" dialog, the background color of the HTML page is not redrawn correctly (leaving white rectangles instead of the background color). How to reproduce ================ 1. open the appended html file (note the grey background) 2. click on "file" link 3. move the "Save As" window over the grey background 4. close the "Save As" window --> parts of the background are drawn white, instead of grey Best, Alex

Dec. 31, 2012

hyphenator valgrind error

by sgeerken＠dillo.org

On Mon, Dec 31, corvid wrote: > ==30983== Invalid read of size 1 > ==30983== at 0x81151B6: fl_utf8back (fl_utf.c:259) > ==30983== by 0x80A2320: dw::fltk::FltkPlatform::prevGlyph(char const*, int) (fltkplatform.cc:599) > ==30983== by 0x809EDA3: dw::Hyphenator::hyphenateSingleWord(dw::core::Platform*, char*, int, lout::misc::SimpleVector<int>*) (hyphenator.cc:327) > ==30983== by 0x809E9F2: dw::Hyphenator::hyphenateWord(dw::core::Platform*, char const*, int*) (hyphenator.cc:256) Good opportunity to get rid of fl_utf8back. I'm working on it, expect a fix next year. Sebastian

Dec. 31, 2012

CAPI ignores download requests when not in cache or handled by the downloads DPI.

by p37sitdu＠lavabit.com

On Sun, Dec 30, 2012 at 05:23:18PM +0000, Jeremy Henty wrote: > > I wondered why "Save link as" silently failed on some of my local web > pages if I had not already visited the link. It turns out they were > "file:///" links and "Save link as" sends a download request which the > CAPI silently ignores if the URL is not cached and is not handled by > the downloads DPI. > > I added a MSG_WARN to log this but haven't changed any behaviour. I > still found it surprising and was wondering if it could be improved? I think the right way to fix it is to redesign downloads DPI so that it doesn't duplicate downloading code. I already said that in Gopher DPI therad. Instead, Dillo could download files and pass them to downloads DPI. Downloads DPI then shows progress and saves files. As I understand, the idea is to have one window for all downloads across many copies of Dillo where progress can be shown and requests to cancel them can be passed to the right copy of Dillo. In the current state only http, https and ftp links are passed to DPI, you can see it in a_Cache_download_enabled.

Dec. 31, 2012

hyphenator valgrind error

by corvid＠lavabit.com

==30983== Invalid read of size 1 ==30983== at 0x81151B6: fl_utf8back (fl_utf.c:259) ==30983== by 0x80A2320: dw::fltk::FltkPlatform::prevGlyph(char const*, int) (fltkplatform.cc:599) ==30983== by 0x809EDA3: dw::Hyphenator::hyphenateSingleWord(dw::core::Platform*, char*, int, lout::misc::SimpleVector<int>*) (hyphenator.cc:327) ==30983== by 0x809E9F2: dw::Hyphenator::hyphenateWord(dw::core::Platform*, char const*, int*) (hyphenator.cc:256) ==30983== by 0x809C087: dw::Textblock::hyphenateWord(int) (textblock_linebreaking.cc:762) ==30983== by 0x809BB6E: dw::Textblock::wordWrap(int, bool) (textblock_linebreaking.cc:609) ==30983== by 0x809B6A9: dw::Textblock::processWord(int) (textblock_linebreaking.cc:441) ==30983== by 0x8098753: dw::Textblock::addText0(char const*, unsigned int, short, dw::core::style::Style*, dw::core::Requisition*) (textblock.cc:1694) ==30983== by 0x8097B02: dw::Textblock::addText(char const*, unsigned int, dw::core::style::Style*) (textblock.cc:1478) ==30983== by 0x8073F5C: Html_process_word(DilloHtml*, char const*, int) (html.cc:1216) ==30983== by 0x807A348: Html_write_raw(DilloHtml*, char*, int, int) (html.cc:3923) ==30983== by 0x80727A9: DilloHtml::write(char*, int, int) (html.cc:531) ==30983== Address 0x661e6bf is 1 bytes before a block of size 16 alloc'd ==30983== at 0x402AC41: malloc (vg_replace_malloc.c:270) ==30983== by 0x80A223F: dw::fltk::FltkPlatform::textToLower(char const*, int) (fltkplatform.cc:584) ==30983== by 0x809E86A: dw::Hyphenator::hyphenateWord(dw::core::Platform*, char const*, int*) (hyphenator.cc:223) ==30983== by 0x809C087: dw::Textblock::hyphenateWord(int) (textblock_linebreaking.cc:762) ==30983== by 0x809BB6E: dw::Textblock::wordWrap(int, bool) (textblock_linebreaking.cc:609) ==30983== by 0x809B6A9: dw::Textblock::processWord(int) (textblock_linebreaking.cc:441) ==30983== by 0x8098753: dw::Textblock::addText0(char const*, unsigned int, short, dw::core::style::Style*, dw::core::Requisition*) (textblock.cc:1694) ==30983== by 0x8097B02: dw::Textblock::addText(char const*, unsigned int, dw::core::style::Style*) (textblock.cc:1478) ==30983== by 0x8073F5C: Html_process_word(DilloHtml*, char const*, int) (html.cc:1216) ==30983== by 0x807A348: Html_write_raw(DilloHtml*, char*, int, int) (html.cc:3923) ==30983== by 0x80727A9: DilloHtml::write(char*, int, int) (html.cc:531) ==30983== by 0x8079D85: Html_callback(int, _CacheClient*) (html.cc:3799)

Dec. 31, 2012

Dpi_connect_socket retry

by jcid＠dillo.org

Hi, On Sat, Dec 29, 2012 at 05:44:22AM +0400, p37sitdu at lavabit.com wrote: > On Thu, Dec 27, 2012 at 04:37:36PM -0300, Jorge Arellano Cid wrote: > > As from the patch you sent (same as before), and the > > explanation you give (ignoring my explanation), I guess you're > > making conclusions out of reading the code in > > Dpi_connect_socket() *only*. > > I made conclusions from reading Dpi_connect_socket() only, because in > the case of error in Dpi_blocking_write file descriptor is left open > and not passed anywhere else. If it was passed somewhere else, I would > read the code there and check if file descriptor was closed there. > > > Have you checked what you state as fact? (i.e. that the FD is > > leaked and not closed by the CCC). FWIW, I did check before > > writing my previous answer. > > Patch that I did for checking is attached. It replaces > Dpi_blocking_write call with -1 to simulate fault in > Dpi_blocking_write for Dpi_connect_socket(). > > Introducing the error in Dpi_blocking_write for all functions doesn't > work, in this case Dpi_connect_socket is not called at all (because > Dpi_start_dpid fails). > > With this patch applied, I recompiled dillo and pressed "bookmarks" > button several times to trigger dpi code. Each time I pressed > "bookmarks" button, new descriptor was leaked: > > % ls /proc/$(pidof dillo)/fd > 0 1 2 3 4 5 > % ls /proc/$(pidof dillo)/fd > 0 1 2 3 4 5 6 > % ls /proc/$(pidof dillo)/fd > 0 1 2 3 4 5 6 > % ls /proc/$(pidof dillo)/fd > 0 1 2 3 4 5 6 7 > % ls /proc/$(pidof dillo)/fd > 0 1 2 3 4 5 6 7 8 > > Then I applied the patch I sent before (with hg qpush) on top of the > fault patch and rebuilt dillo: "make clean && make". I ran dillo again > with ./src/dillo and did the same test with bookmarks button: > > % ls /proc/$(pidof dillo)/fd > 0 1 2 3 4 5 > % ls /proc/$(pidof dillo)/fd > 0 1 2 3 4 5 > % ls /proc/$(pidof dillo)/fd > 0 1 2 3 4 5 > diff -r 8e10859b2aab src/IO/dpi.c > --- a/src/IO/dpi.c > +++ b/src/IO/dpi.c > @@ -641,7 +641,7 @@ > /* send authentication Key (the server closes sock_fd on error) */ > } else if (!(cmd = a_Dpip_build_cmd("cmd=%s msg=%s", "auth", SharedKey))) { > MSG_ERR("[Dpi_connect_socket] Can't make auth message.\n"); > - } else if (Dpi_blocking_write(sock_fd, cmd, strlen(cmd)) == -1) { > + } else if (-1 == -1) { > MSG_ERR("[Dpi_connect_socket] Can't send auth message.\n"); > } else { > ret = sock_fd; Ah, OK, now I see what you mean. There's a semantic ambiguity, the comment: /* send authentication Key (the server closes sock_fd on error) */ means: "if there's an error with the authentication" (I know because I wrote the comment). (and as the patch removed this comment) I made the same test but with: - } else if (!(cmd = a_Dpip_build_cmd("cmd=%s msg=%s", "auth", SharedKey))) { + } else if (!(cmd = a_Dpip_build_cmd("cmd=%s msg=%s","auth",SharedKey+1))) { and the CCC closes the file descriptor as expected. The error case you noticed exists, and although highly unlikely to happen, requires some thought. As from the test above, the socket is connected, then nothing is written to simulate an error, but the OS never sees an error, as it doesn't exist, and doesn't notify either end (IOW, it leaves the socket passively expecting for data to come). Now, the canonical way to handle the whole process should have been to build the CCC upon connect(), then try to authenticate, and let the CCC handle any errors that may come, but this is probably too much code for little gain, so it was all coded in Dpi_connect_socket(). The problem with closing FDs outside the CCC is that it can introduce race conditions, but in this case the CCC is partially built, so your patch suggestion may be OK. I'll have to give it some more thought and tests... -- Cheers Jorge.-

Dec. 30, 2012