126417 - Can't use streamconverter chaining more than once per session

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[Bradley Baetz (:bbaetz)]

The stream converter service supports chaining, but it calls BuildGrpah each
time, so entries get duplicated for the BFS, and things fail.

I tried just returning early from BuildGraph, but the code seems to do something
to the hashtables, and so the counts are differnet, and so the lBFSTable.Count()
== vertexCount assertion fires.

Calling reset fixes that problem, but then we crash because SCTableData relies
on a copy constructor for the hashtable keys, but they don't have one, so you do
a bitwise copy, and then when Reset destroys the keys, you end up with dangling
pointers.

I've hacked up TestStreamConv to show this bug.

Valeski, I believe you wrote this originally. Is the patch I'm going to attach
the right way to do this?

This blocks one of my 0.9.9 bugs, although I can work arround it by manually
chaining.

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: hack

This has all my debugging hacks in it, of course. there has to be a better
solution....

Comment 2

[Judson Valeski]

what bug is this blocking for you?

Comment 3

[Bradley Baetz (:bbaetz)]

Bug 110760 - I'm moving the listings back into necko, and so I convert from
ftp-* to text/html, which needs to go via application/http-index-format.

Since I know the path I need, I can probably just do the chaining manually, so
its not strictly a blocker.

Without this fix, my patch will crash on the second directory listing seen in
the session.

I'm happy with the workarround for 0.9.9, given how close we are to the
milestone, if you think that there is a better fix. Otherwise I'll just tidy
mine up and try to get it in first.

Comment 4

[Judson Valeski]

yea, I built this thing w/ the assumption that if you know the direct path,
you'd just do that by hand. let's go w/ that workaround.

Comment 5

[Bradley Baetz (:bbaetz)]

OK. But we do have a broken feature which noone else uses then ;)

Comment 6

[Judson Valeski]

I guess I don't understand the bug here. stream converters are used many times
in a single session but we don't crash.

Comment 7

[Judson Valeski]

chaining used to work just fine (that was the only reason I wrote this thing),
I'm not sure what could be broken here. I don't see the duplicated entries
AddAdjacency() protects against dupes.

Comment 8

[Bradley Baetz (:bbaetz)]

The streamconverterService has an optimisation where if you want to go from
foo/a to foo/b, and there exists an exact match, it won't build the graph, so we
never reach the problem code.

However, if you have two converters, foo/a->foo/b and foo/b->foo/c, then trying
to go from foo/a->foo/c via AsyncConvertData will build the graph. Then if you
do this again, you run into the problems in my initial comment (which was
written at 3am, and could be a bit clearer, I'll admit. Let me try again):

The graph building uses a couple of hashtables, and if you rebuild the graph
again you get double entries. However, some entries have been 'used', and so
they're not entered again, and the the two hash tables have different sizes, so
we hit the assertion.

Clearing the table fixes that, except that the act of clearing frees memory that
the hask table doesn't own, because the hash tbale code uses a copy constructor
for the key. This doesn't work, because the compiler uses its builtin one (there
is no explict copy constructor), so it just does a bitwise copy of the actual
pointer+flags. When its destroyed, it frees the memory, and then reconstructing
the table tries to free the same pointer again, which crashes. I fixed that by
calling clone in the constructor for our keys.

Comment 9

[Bradley Baetz (:bbaetz)]

AddAdjacency doesn't protect against dupes, because the state is different once
we've run the BFS through once.

I know this used to work - I'm not sure what broke it. cvs log didn't show
anything useful.

Comment 10

[Doug Turner (:dougt)]

bradley, if you only call BuildGraph() from Init(), will this solve the problem?

Comment 11

[Judson Valeski]

I think this goes away w/ the fix to
http://bugzilla.mozilla.org/show_bug.cgi?id=73308

Comment 12

[Boris Zbarsky [:bzbarsky]]

Bradley, is this still an issue?

Comment 13

[Bradley Baetz (:bbaetz)]

I have no idea :)

Comment 14

[Darin Fisher]

bbaetz: are you saying that you don't intend to investigate this?  if so, then
we should probably find another owner for this bug, right? ;)

Comment 15

[Bradley Baetz (:bbaetz)]

Theres not much chance of me finding time to investigate this, sorry.