31581 - Long multipart/mixed with table is truncated/doesn't display (was: Bugzilla submit query does not complete)

Status: VERIFIED FIXED

(Core :: Networking, defect, P2)

Description

[Alan S. Jones]

Using win32 2000031115 build on Win95

Submitting a query in Bugzilla never completes.  It says "please stand by", then 
the whole screen goes white.  

The bottom status line say "Transferring data from bugzilla" and that is it.

Comment 1

[rods (gone)]

eric, could you take a look?

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I see this on Linux too.  Changing severity to blocker because it blocks dogfood
use of the browser.

The Bugzilla query page uses multipart/x-mixed-replace.  I suspect that's the
problem, and not the form submission.  When I load a query, I see:

WEBSHELL+ = 4
Opening file DistinguishedSchema.tbl failed
Opening file FieldSchema.tbl failed
Opening file URLFieldSchema.tbl failed
Opening file SchemaConcat.tbl failed
Opening file 51491103.u failed
WEBSHELL- = 3
nsWidget::~nsWidget() of toplevel: 87 widgets still exist.
nsWidget::~nsWidget() of toplevel: 82 widgets still exist.
nsWidget::~nsWidget() of toplevel: 77 widgets still exist.
nsWidget::~nsWidget() of toplevel: 52 widgets still exist.
nsWidget::~nsWidget() of toplevel: 47 widgets still exist.
nsWidget::~nsWidget() of toplevel: 42 widgets still exist.
nsWidget::~nsWidget() of toplevel: 37 widgets still exist.
nsWidget::~nsWidget() of toplevel: 32 widgets still exist.
nsWidget::~nsWidget() of toplevel: 27 widgets still exist.
nsWidget::~nsWidget() of toplevel: 22 widgets still exist.
nsWidget::~nsWidget() of toplevel: 17 widgets still exist.
nsWidget::~nsWidget() of toplevel: 12 widgets still exist.
Error loading URL http://bugzilla.mozilla.org/query.cgi 
Document: Done (4.374 secs)

If I then hit reload, I see:

Going to reload
###!!! ASSERTION: NS_ENSURE_TRUE(NS_SUCCEEDED(InternalLoad(mCurrentURI,
mReferrerURI))) failed: '(!((InternalLoad(mCurrentURI, mReferrerURI)) &
0x80000000))', file nsDocShell.cpp, line 976
###!!! Break: at file nsDocShell.cpp, line 976

cc:ing travis because he landed major changes to DocShell/WebShell around when
this broke (which are therefore a possible cause of this problem), and valeski
because he knows multipart/replace stuff (from the network end of things).

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I forgot to say that I'm seeing it in a Linux debug build from yesterday (03-12)
morning and a Linux opt build from yesterday (03-12) evening.  The snips of
output above are from the former.

Comment 4

[Jason Eager]

This shows up on my browser as well, Linux build 2000031315.

Furthermore, if I try to view source, all I get is a black window where
the source should appear.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Adding regression and dogfood keywords.  Is anyone looking at this?

Comment 6

[Eric Pollmann]

Well, this is certainly a fun one.  I have reduced this down quite a bit, but it
seems that the following conditions must all be met:

1) Multipart/mixed
2) Contains table
3) Long document
4) Table gets split into two packets

If all of the above are met, the page will not display

In fact, if the document is just the right length, the page will display, but
will be truncated (end prematurely).

CC'ing Chris Karnaze (table guru), and Harish (parser guru), and reassigning bug
to Judson since Netlib is the lowest level here.

There is a reproducible test case inside the firewall at this URL:

http://blueviper.mcom.com/cgi-bin/multipartlong.cgi

I'll attach the perl source for the CGI, but note that it depends on the size
packets that your web server sends out, so you may have to tweak the number of
lines of "Foo" (2Kb each) to get the bug to present itself on any given web
server.

Comment 7

[Eric Pollmann]

Attachment: Test case perl source (may need to tweak for your webserver)

Comment 8

[Eric Pollmann]

Netlib or the parser *may* be involved here because the content model does not
contain the table at all - the document has been truncated.

The content sink *must* be involved here because despite the content model
containing a text node of "Foo"'s, no corresponding frame is created, and thus
nothing is displayed.

But this is pure conjecture, I haven't stepped though the code yet...  :)

Comment 9

[leger]

rick to check into this bug.

Comment 10

[leger]

Putting on the PDT+ radar for beta1.   Need to assess risk to fix and get 
approval.

Comment 11

[Judson Valeski]

over to travis, necko hasn't changed and this looks like a consumer issue. who 
put beta1 regression on this? is someone seeing this in beta1 builds?

Comment 12

[harishd]

Rickg and I see only a part of the document data being handed over to the 
parser.  

FYI: We did not come across a TABLE tag at all(!)...all we saw was a bunch of 
"foo"s..

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I marked it dogfood because nobody had commented on the bug and I considered it
very important.  It's not marked beta1.

Has anybody seen this on the beta1 branch?  I have not (but I haven't checked
either).

Comment 14

[don]

Clearing PDT+ for re-consideration.  Why was this marked as a beta blocker??? 
And why on earth does Travis have this bug?

Comment 15

[travis]

mscott is working on a patch for this.

Comment 16

[leger]

Putting on the PDT+ radar for beta1.  Assigning to mscott and putting travis on 
cc.

Comment 17

[Judson Valeski]

not sure why this is PDT, it's not a problem on the branch.

Comment 18

[Scott MacGregor]

I'm running the beta branch builds and this works fine. As judson pointed out, I
think PDT got confused with this bug. It's a tip problem not a branch problem.

Removig PDT+ resolution so they can re-evaluate.

Comment 19

[Scott MacGregor]

Hey Judson/Travis. I don't think the uri loader changes we talked about before
(via email) are the right way to solve this problem.

I made those changes in my tree and it just led to further problems. In
particular, the parser is the stream listener on the other end of the uri
loader. I allowed the uriloader to make multiple onstart and on stop requests
per our discussion. But the parser isn't set up for this at all! I got several
assertions and then a crash in the parser when I called on start request
multiple times.

This fact and the fact that this used to work just fine with the uri loader only
issuing one on start / on stop pair before makes me think that we are trying to
solve the wrong part of the problem.

I'm not sure where to start looking yet.

Comment 20

[Daniel (Leaf) Nunes]

Ok, marking a smoketest blocker. querying bugzilla is kind of important for
everyday work.

Comment 21

[Akkana Peck]

Oddly, my bookmark for "my M15 bugs" works:

http://bugzilla.mozilla.org/buglist.cgi?bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&email1=akkana&emailtype1=substring&emailassigned_to1=1&email2=&emailtype2=substring&emailreporter2=1&changedin=&chfieldfrom=&chfieldto=Now&chfieldvalue=&target_milestone=---&target_milestone=M13&target_milestone=M14&target_milestone=M15&short_desc=&short_desc_type=substr&long_desc=&long_desc_type=substr&bug_file_loc=&bug_file_loc_type=substr&status_whiteboard=&status_whiteboard_type=substr&newqueryname=&form_name=query&order=bugs.target_milestone%2C%20bugs.priority%2C%20bugs.bug_severity

but putting akkana in the assigned-to and M15 in the milestone field then
clicking submit produces a blank page:

http://bugzilla.mozilla.org/buglist.cgi?bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&email1=akkana&emailtype1=substring&emailassigned_to1=1&email2=&emailtype2=substring&emailreporter2=1&bugidtype=include&bug_id=&changedin=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&target_milestone=M15&short_desc=&short_desc_type=substring&long_desc=&long_desc_type=substring&bug_file_loc=&bug_file_loc_type=substring&status_whiteboard=&status_whiteboard_type=substring&keywords=&keywords_type=anywords&field0-0-0=noop&type0-0-0=noop&value0-0-0=&cmdtype=doit&newqueryname=&order=Reuse+same+sort+as+last+time 

Comment 22

[travis]

Smoke tests blocker?  Doesn't that mean the tree is blocked for this?  The tree 
should not be blocked for this.  This does not prevent people from by and large 
testing.  Sure, this regresses the product, but for fall back, we have the 
beta.  We have way too much to get done between now and beta 2 to block the 
tree for any dogfood regression.  That stops way too many people.

Comment 23

[Daniel (Leaf) Nunes]

removing smoketest keyword... but i'm going to be hounding the owner of this bug
to get it fixed quickly, since i have a fondness for looking at my buglist.

Comment 24

[Akkana Peck]

Not being able to use bugzilla to look at buglists is a pretty serious
regression, especially this has been broken all this week.

Comment 25

[travis]

akkana agreed, but there are people looking at it.  It's not serious enough to 
stop everyone's development for it.

Comment 26

[Phil Peterson]

PDT+ trunk only. not related to beta1

Comment 27

[Scott MacGregor]

Hey Travis, we may need to start looking at the webshell landing you had on
Friday and trying to figure out from those changes how this broke (that's when
this broke). I don't think the changes we talked about to the uriloader to do
this are going to pan out.

Comment 28

[Scott MacGregor]

Unfortunately it appears to be timing related. Every time I step through the 
load in the debugger, it works just fine! But running all the way through always 
fails. Someone checked in something last Friday during the carpools that exposed 
a timing problem in here apparently.

Comment 29

[Scott MacGregor]

I think I may have found the problem. You'll love this one. In
nsMultiMixedConv::BuildUrl we've got the following code snippet:
    nsCAutoString dummyURIStr(uriSpec);
    dummyURIStr.Append("##");

dummyURIStr.Append(mPartCount);

mPartCount was a PRInt32 and referred to the part number we were interested in.
so we are appending ##mPartCount to the end of the url in order to create a new
part url that layout would turn around and load.

Unforunately, mPartCount was invoking the version of nsString::Append which
takes a PRUnichar. So mPartCount was implicitly getting converted to a unichar.
In some cases this was causing nothing to get appended to the url. so you'd get
## at the end of the url without a part number!! So we wouldn't load that
particular part.

I changed the code to force us to use the version of append with takes a PRInt32:
dummyURIStr.Append((PRInt32) mPartCount, 10 /* radix */);

Now all the part urls are properly formed.

After this one line change, I have not been able to reproduce any problems with
bugzilla. Everything (including submission of this bug report) is working great.

Comment 30

[Scott MacGregor]

I've checked in the one line fix mentioned above. I'm now happily using bugzilla
again on the tip.

Comment 31

[travis]

Hey, GREAT WORK mscott!  So what exposed this?  Did that line change when the 
string API code was recently changed?  If so it's something we should note to 
watch out for as I know there will be more string changes coming in the near 
future.  So what is the outcome of how it is suppose to work?  Do two loads 
actually occur, thus two DoContents getting called?  Or are we actually reusing 
the same content viewer?

Comment 32

[ckritzer (gone)]

Marking VERIFIED FIXED on:
- Linux6 2000-03-17-06 Commercial nb1n build
- MacOS9 2000-03-16-16 Commercial nb1n build
- Win98 2000-03-16-06 Commercial nb1n build