- Dillo-dev - lists.mailman3.com

[BUG]: Redirect to previous page + Reload crashes

by onepoint＠starurchin.org

On Sun, May 25, 2008 at 12:31:51AM +0000, corvid wrote: > Since it looks like Nav_stack_truncate() already got rid of > everything at stack_idx and beyond, maybe Nav_stack_insert() could > be replaced by a Nav_stack_append() function. Good point! And there is no need for Nav_stack_insert() to update bw->nav_stack_ptr . In fact it *mustn't* do that because the code that calls Nav_stack_insert() assumes that bw->nav_stack_ptr *won't* change and increments it itself. Here's a patch that replaces Nav_stack_insert() by Nav_stack_append() as you suggest. Regards, Jeremy Henty

May 25, 2008

remaining preferences

by jcid＠dillo.org

On Sat, May 24, 2008 at 07:28:47PM +0000, corvid wrote: > # Dicache is where the Decompressed Images are cached (not the original ones). > # If you have a lot of memory and a slow CPU, use YES, otherwise use NO > use_dicache > > The code always behaves like use_dicache=NO > > /* > * Unrefs the counter of a dicache entry, and _if_ no DwImage is acessing > * this buffer, then we call Dicache_free to do the dirty job. > */ > void a_Dicache_unref(const DilloUrl *Url, int version) > { > DICacheEntry *entry; > > if ((entry = Dicache_get_entry_version(Url, version))) { > /*if (--entry->RefCount == 0 && (entry->next || !prefs.use_dicache)) {*/ > if (--entry->RefCount == 0) { > Dicache_remove(Url, version); > } > } > } > > Perhaps this was done for a reason. Yes, I wrote cache/dicache code before imgbuf was introduced. The dicache showed impressive performance when clicking Back/Forward on pages with images because they were decoded once and the RGB data kept in the dicache. Time showed it wasn't as useful as it seemed because memory usage was to high and usually machines with big memory came with a big CPU too. Probably the dicache is obsolete and can be completely removed now. We left that task pending because we had yet a lot to do after images were enabled in dillo2. > # If you prefer oblique over italic fonts, uncoment next line > use_oblique > > FLTK doesn't know about oblique. I don't know whether anybody cares enough > to change that. I don't. Cleanup patch welcomed. ;) > # Show tooltip popup for images? > # Note: We use the "title" attribute and not "alt". > # More info at: http://bugzilla.mozilla.org/show_bug.cgi?id=25537 > show_tooltip > > I did the easy part of tooltips long ago, but I haven't been able to > care enough to want to do the fltk part. > > > # Set this to YES, if you want to limit the word wrap width to the vieport > # width (may be useful for iPAQ) > limit_text_width > > It doesn't do anything. I don't think it's ever done anything. > It sure would be nice if it did something. But I don't have the expertise > to change that. For instance: Some times HTML pages for mailing lists render one looong line for a message. The MUA packs the message as a single line and the mail2html filter wraps it with PRE. AFAIR the SPEC said original formatting was to be honoured in PRE, but browsers, in practice, tend to wrap it. Sometimes you need one behaviour and then another. > # Here you can choose to hide some widgets of the dillo panel... > show_menubar > > Not currently used. Meaningless with the current panel-cycling with > right click on the search button (which I imagine is not going to be > the permanent way to do that). Not the permanent way, but quite handy! ;) > # Enabling this will restrain OpenUrl and FindText, but may be required > # for the ION window manager. > transient_dialogs > > from fltk::Window::child_of(): > Tell the system that this window will not have an icon, it will > dissappear and reappear when \a parent is iconized or shown, and it > is forced to always be above \a parent. On X this is called a > "Transient window", and Windows calls this a "overlapping child". \a > parent is different than the parent(), which must be zero. > (and down in fltk::CreatedWindow::create, there's a > if (window->child_of() && window->child_of()->shown()) > XSetTransientForHint(xdisplay, x->xid, window->child_of()->i->xid);) > > If Justus gets rid of the dialogs, obviously the pref can be removed. > Enabling this means making our own dialogs for message(), choice(), input(). Somebody using ION may check this. > # When filling forms, our default behaviour is to submit on enterpress, > # but only when there's a single text entry (to avoid incomplete submits). > # OTOH, if you have to fill the same form lots of times, you may find > # useful to keep away from the mouse by forcing enter to submit. > enterpress_forces_submit > > The code currently always submits on enter. I like it that way and > didn't realize that it wasn't supposed to. It depends on the FORM, and user. I'd like to hav it working. -- Cheers Jorge.-

May 25, 2008

patch: Re: [Dillo-dev] Fuzzing dillo

by corvid＠lavabit.com

Justus wrote: > I was playing around with Michal Zalewskis html fuzzer 'mangle' trying > to crash dillo and succeeded :). Furthermore I was able to reduce the > ~100k page that was triggering a NULL pointer dereferencing to three > tags (see the attached page). > > Some digging around (html.cc is a monster ^^) revealed that > > style_attrs.setBorderColor ( > Color::createShaded(HT2LT(html), > style_attrs.backgroundColor->getColor())); > > in Html_tag_open_table was failing due to the fact that > style_attrs.backgroundColor was NULL. > > Html_tag_open_select sets the backgroundColor to NULL and this is > inherited by the table tag since it's a child of the select tag. I doubt I realized you can try to put things like tables inside selects back when I wrote that. Maybe Jorge will know something easy and general that can be done based on parse modes to keep dillo from wasting its time calling things like Html_tag_open_table that aren't going to be displayed anyway, but the attached patch at least fixes the crash. > Greping through the code shows that there are at least two other > functions that set backgroundColor (and color) to NULL > (Html_tag_open_input and Html_tag_open_isindex) and twelve locations > calling getColor() on either color or backgroundColor. I think those are all right because they aren't allowed to contain anything, although, as I hinted above, my knowledge of the parsing code is not really detailed. > Furthermore I'd suggest to break down html.cc into several files. It is > about 6000 lines long (-> hard to dig into) and it takes gcc roughly > five seconds to compile on my box. I'd like this as well.

May 25, 2008

Fuzzing dillo

by jcid＠dillo.org

On Sun, May 25, 2008 at 01:12:38PM +0200, Justus Winter wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Hi :) > > I was playing around with Michal Zalewskis html fuzzer 'mangle' trying > to crash dillo and succeeded :). Good. It's no surprise because dillo2 needs a careful review of behaviour when facing strange/malicious values. Obviously this phase was procrastinated until basic functionality was completed. For instance, most probably you'll get to crash dillo2 by passing it some negative values in attributes. The problem of being robust when parsing garbage or malicious code needs a general strategy. I was delighted to read DJB's paper: "Some thoughts on security after ten years of qmail 1.0" (Strongly suggested to everyone!). There're lots of good ideas there, tried and true. It's clear that implementing them all is a huge task, but just a couple of them can make wonders, and reading the paper makes you think in a different way when designing code. For instance, in dlib, I took the simple approach of checking each function's input before processing (not 100% complete, but you get the idea), and it has proven rock solid. The extra time spent on checks doesn't impact performance noticeably, but the simplicity and joy of relying on a strong ADT set is priceless. Another strategy that allowed me to simplify dillo a lot, was to avoid multiple execution paths. With multiple paths, code gets much more complex, and maintenance is even harder, minor changes have side effects to track and testing depends on the path taken. A single path is tested by common use all the time, has only one place to make changes, is simpler to understand/debug/modify, and the simplicity leverage clearly overcomes the possible performace tradeoff. Beautifuly expressed: "The key to performance is elegance, not battalions of special cases." Jon Bentley and Doug McIlroy e.g. The CCC used to have one-branch-per-protocol. When it was switched to a single "universal" branch, CCC code size was reduced importantly, a lot of the complexity vanished, fixing a bug benefited all protocols, there were no multiple paths to track, etc. e.g.2 Memory handling in dillo. The general idea of trying to make each function deallocate what it allocated, made a hell of memory traceback debugging vanish. And the performance impact was unnoticed! > Furthermore I was able to reduce the > ~100k page that was triggering a NULL pointer dereferencing to three > tags (see the attached page). > > Some digging around (html.cc is a monster ^^) revealed that > > style_attrs.setBorderColor ( > Color::createShaded(HT2LT(html), > style_attrs.backgroundColor->getColor())); > > in Html_tag_open_table was failing due to the fact that > style_attrs.backgroundColor was NULL. > > Html_tag_open_select sets the backgroundColor to NULL and this is > inherited by the table tag since it's a child of the select tag. > > Greping through the code shows that there are at least two other > functions that set backgroundColor (and color) to NULL > (Html_tag_open_input and Html_tag_open_isindex) and twelve locations > calling getColor() on either color or backgroundColor. > > I have no idea how to _properly_ fix this issue. Not me either. I'd check what NULL means. A quick look hints towards a non inherited value: /** * \brief Reset those style attributes to their standard values, which are * not inherited, according to CSS. */ void StyleAttrs::resetValues () A default backgroundColor should be a good place to start. HTH. > > Furthermore I'd suggest to break down html.cc into several files. It is > about 6000 lines long (-> hard to dig into) and it takes gcc roughly > five seconds to compile on my box. I agree. The hard part is a good choice on what to separate. ;) -- Cheers Jorge.-

May 25, 2008

Q: why does Nav_stack_clean() free the data and Nav_stack_truncate() not?

by jcid＠dillo.org

Hi Jeremy, On Sat, May 24, 2008 at 08:15:47PM +0100, Jeremy Henty wrote: > > While reviewing nav.c to figure out the reload crash bug I noticed > something that seems inconsistent: both Nav_stack_truncate() and > Nav_stack_clean() remove items from the navigation stack, yet > Nav_stack_truncate() does not free the data it removes and > Nav_stack_clean() does. Is this a bug? If not, what haven't I > understood? Nav_stack_truncate() and Nav_stack_insert() are ad-hoc bindings. They're called just in one place of the code. They're not clean well-defined functions of an ADT. This is "your" mistake. :-) I stumbled upon Nav_stack_clean() some time ago, and wondered whether it still serves a clear purpose or if it could be replaced. The nav.c module is asking for a clean ADT, and if you can work on it, it's certainly welcomed. Beware that it's more complex than it looks. Especially the repush code. Nav.c takes care of redirections, reloads, repushes, expects, jumps, pushes, etc. BTW, if it's to be rewritten it'd be good to have the TABs interface in mind. This is, if we end with a clean ADT or object, we could have an array of them, one for each tab, and switch freely among navigation stacks. -- Cheers Jorge.-

May 25, 2008

[patch] dw2 memory reduction

by jcid＠dillo.org

On Sun, May 25, 2008 at 10:08:12AM +0200, Johannes Hofmann wrote: > Hi, > > On Sat, May 24, 2008 at 04:39:42PM -0400, Jorge Arellano Cid wrote: > > On Sat, May 24, 2008 at 02:58:08PM +0200, Johannes Hofmann wrote: > > > Hi, > > > > > > attached is patch to reduce the memory usage of dw2: > > > > Excellent! > > (AFAIU this is near 30% memory usage reduction in dw2) > > > > Committed. > > > > > > > > * Styles are now reused if they are equal. This brings down the > > > number of Style objects on the mysql page from around 180000 to > > > 23000 and reduces memory usage by about 25M at the cost of an > > > additional HashTable. > > > Maybe the hash table should be part of fltkplatform > > > as the hash tables for Color and Font? > > > There would be even more sharing of styles possible, if > > > x_link would not be part of style. On the other hand about 3M for > > > styles on the huge mysql page is pretty ok. > > > > > > * Size of struct Word is reduced by 4 integers by using shorts. I > > > know that this is questionable, but I think in the case of struct > > > Word that can be allocated a lot it is worth the hassle. > > > Please check whether my assumption that space lengths and word > > > lengths can be stored in shorts are valid in general. > > > > It looks OK at first sight, but to be sure it'd be good to know > > what happens when the limit is surpassed. > > Just tried it on a page with 40000 char long word. With short for > hlStart/hlEnd, selection is - obviously - no longer possible. > That's clearly a regression. > Please revert with attached patch. A max of 32K for a single word, looks quite reasonable to me. I meant, if the browser doesn't crash, and fails gracefully, why not. BTW, it can be extended to 64K with unsigned, by using 65535 as sentinel value (instead of -1), but again, is it worth? It'd be good to know how much "struct Word" is reduced in size (after compiler alignment), and how much it impacts overall memory usage. I mean if struct word is reduced 20% and that yields a 4% overall memory reduction, it may be not worth complicating the data type. OTOH, ... > > BTW, "unsigned short" looks like a good idea for origSpace and effSpace. Committed the "unsigned short" part. -- Cheers Jorge.-

May 25, 2008

Fuzzing dillo

by 4winter＠informatik.uni-hamburg.de

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi :) I was playing around with Michal Zalewskis html fuzzer 'mangle' trying to crash dillo and succeeded :). Furthermore I was able to reduce the ~100k page that was triggering a NULL pointer dereferencing to three tags (see the attached page). Some digging around (html.cc is a monster ^^) revealed that style_attrs.setBorderColor ( Color::createShaded(HT2LT(html), style_attrs.backgroundColor->getColor())); in Html_tag_open_table was failing due to the fact that style_attrs.backgroundColor was NULL. Html_tag_open_select sets the backgroundColor to NULL and this is inherited by the table tag since it's a child of the select tag. Greping through the code shows that there are at least two other functions that set backgroundColor (and color) to NULL (Html_tag_open_input and Html_tag_open_isindex) and twelve locations calling getColor() on either color or backgroundColor. I have no idea how to _properly_ fix this issue. Furthermore I'd suggest to break down html.cc into several files. It is about 6000 lines long (-> hard to dig into) and it takes gcc roughly five seconds to compile on my box. Justus - -- gpg key fingerprint: C82D 382A AB38 1A54 5290 19D6 A0F9 B035 686C 6996 -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFIOUmmoPmwNWhsaZYRAprIAJ9X9HSzc4Isx+NyFWxlfSFPrwj/VACdF8GD x67scPvD6FCrOezl69NbaHA= =z0iv -----END PGP SIGNATURE-----

May 25, 2008

[patch] dw2 memory reduction

by Johannes.Hofmann＠gmx.de

Hi, On Sat, May 24, 2008 at 04:39:42PM -0400, Jorge Arellano Cid wrote: > On Sat, May 24, 2008 at 02:58:08PM +0200, Johannes Hofmann wrote: > > Hi, > > > > attached is patch to reduce the memory usage of dw2: > > Excellent! > (AFAIU this is near 30% memory usage reduction in dw2) > > Committed. > > > > > * Styles are now reused if they are equal. This brings down the > > number of Style objects on the mysql page from around 180000 to > > 23000 and reduces memory usage by about 25M at the cost of an > > additional HashTable. > > Maybe the hash table should be part of fltkplatform > > as the hash tables for Color and Font? > > There would be even more sharing of styles possible, if > > x_link would not be part of style. On the other hand about 3M for > > styles on the huge mysql page is pretty ok. > > > > * Size of struct Word is reduced by 4 integers by using shorts. I > > know that this is questionable, but I think in the case of struct > > Word that can be allocated a lot it is worth the hassle. > > Please check whether my assumption that space lengths and word > > lengths can be stored in shorts are valid in general. > > It looks OK at first sight, but to be sure it'd be good to know > what happens when the limit is surpassed. Just tried it on a page with 40000 char long word. With short for hlStart/hlEnd, selection is - obviously - no longer possible. That's clearly a regression. Please revert with attached patch. > > BTW, "unsigned short" looks like a good idea for origSpace and effSpace. Adjusted in patch. Cheers, Johannes

May 25, 2008

[BUG]: Redirect to previous page + Reload crashes

by corvid＠lavabit.com

Jeremy wrote: > On Sat, May 24, 2008 at 08:04:19PM +0100, Jeremy Henty wrote: > > > Reviewing the rest of nav.c I think there is a similar potential > > problem with Nav_stack_insert() , which also modifies bw->nav_stack > > . > > On second thoughts, Nav_stack_insert() seems broken. It frees the > pointer at the insertion point, so it clearly expects the new item to > replace the old. But then it calls dList_insert_pos() which does > *not* replace the item at the insertion point, instead it extends the > list and moves everything up to make space. So now we have a freed > pointer in bw->nav_stack . > > Either Nav_stack_insert() should not be freeing the pointer or it > should call a dList_*() function that replaces the item it has freed. > Which? Since it looks like Nav_stack_truncate() already got rid of everything at stack_idx and beyond, maybe Nav_stack_insert() could be replaced by a Nav_stack_append() function.

May 25, 2008

[BUG]: Redirect to previous page + Reload crashes

by onepoint＠starurchin.org

On Sat, May 24, 2008 at 08:04:19PM +0100, Jeremy Henty wrote: > Reviewing the rest of nav.c I think there is a similar potential > problem with Nav_stack_insert() , which also modifies bw->nav_stack > . On second thoughts, Nav_stack_insert() seems broken. It frees the pointer at the insertion point, so it clearly expects the new item to replace the old. But then it calls dList_insert_pos() which does *not* replace the item at the insertion point, instead it extends the list and moves everything up to make space. So now we have a freed pointer in bw->nav_stack . Either Nav_stack_insert() should not be freeing the pointer or it should call a dList_*() function that replaces the item it has freed. Which? Regards, Jeremy Henty

May 25, 2008