169980 - document.load() needs to add itself to loadgroup

Status: RESOLVED FIXED

(Core :: XML, defect, P2)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Same as bug 83573, for XML document now.

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Potential fix

This patch seems to work, but for some reason it gives these assertions when
using the testcase:

###!!! ASSERTION: parser needs a content type to find a dtd: 'Not Reached',
file
 c:/builds/mozilla/htmlparser/src/nsParser.cpp, line 1954
WARNING: Content has no document., file
c:/builds/mozilla/layout/html/base/src/n
sTextFrame.cpp, line 5302
WARNING: Reflow of frame failed in nsLineLayout, file
c:/builds/mozilla/layout/h
tml/base/src/nsLineLayout.cpp, line 1053
###!!! ASSERTION: reflow dirty lines failed: 'NS_SUCCEEDED(rv)', file
c:/builds/
mozilla/layout/html/base/src/nsBlockFrame.cpp, line 950
nsBlockReflowContext: Block(p)(3)@03D35524 metrics=-559038737,-559038737!
nsBlockReflowContext: Block(p)(3)@03D35524 didn't set whad
-559038737,-559038737
,-559038737,-559038737!
###!!! ASSERTION: reflow dirty lines failed: 'NS_SUCCEEDED(rv)', file
c:/builds/
mozilla/layout/html/base/src/nsBlockFrame.cpp, line 950
nsBlockReflowContext: Block(body)(2)@03D34F40 metrics=-559038737,-559038737!
nsBlockReflowContext: Block(body)(2)@03D34F40 didn't set whad
-559038737,-559038
737,-559038737,-559038737!
###!!! ASSERTION: reflow dirty lines failed: 'NS_SUCCEEDED(rv)', file
c:/builds/
mozilla/layout/html/base/src/nsBlockFrame.cpp, line 950

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Harish, can you check out why we get that assert in the parser and if it is
something we need to be concerned before checkin this in?

Comment 3

[harishd]

Looks like the GetContentType(), on tehe channel, is returning NS_FAILURE.

Comment 4

[harishd]

According to the assertion if there was no content type then we may not have a
DTD. That is not completely true, at least for the time being, because when the
content-type is unavailable the parser finds an "appropriate" DTD by scanning
the buffer content. IMO, the code that does the content snooping should go away
because it might lead to DTD-SINK mismatch.

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 118821 [details] [diff] [review]
Fix

This patch adds document.loaded document to the currently visible document's
loadgroup, so that if you have an asynchronous document.load in progress and
you leave the current page, the pending load is aborted and no load event
listeners will be called. This implementation follows what we did for
XMLHttpRequest.

The only thing that I was concerned about was the assertion in the parser.
After talking to Darin and looking at the code a bit more I think it is safe to
remove; the GetContentType() on the channel will naturally fail because the
load was aborted by leaving the page. The rest of the parser seems to work
fine: there is no OnDataAvailable() call, and the parser's OnStopRequest() has
code in preparation for for that case - we create an empty HTML document which
is fine even here.

Comment 7

[harishd]

Comment on attachment 118821 [details] [diff] [review]
Fix

>Index: htmlparser/src/nsParser.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/htmlparser/src/nsParser.cpp,v
>retrieving revision 3.322
>diff -u -r3.322 nsParser.cpp
>--- htmlparser/src/nsParser.cpp	15 Mar 2003 01:02:48 -0000	3.322
>+++ htmlparser/src/nsParser.cpp	29 Mar 2003 00:24:17 -0000
>@@ -1969,8 +1969,6 @@
>   {
>     mParserContext->SetMimeType(contentType);
>   }
>-  else
>-    NS_NOTREACHED("parser needs a content type to find a dtd");

Why did you remove this?

> #include "nsIDOMProcessingInstruction.h"
> #include "nsIDOMDocumentType.h"
>@@ -376,6 +377,30 @@
>   return nsXPointer::Evaluate(this, aExpression, aResult);
> }
> 

>+  // nsIRequest::LOAD_BACKGROUND prevents throbber from becoming active,
>+  // which in turn keeps STOP button from becoming active  
>+  rv = NS_NewChannel(getter_AddRefs(channel), uri, nsnull, loadGroup, this, 
>+                     nsIRequest::LOAD_BACKGROUND);

I'm not very clear here. Are you saying that we cannot stop the load?

Comment 8

[Darin Fisher]

harish: see heikki's comments... he describes why the NS_NOTREACHED is bogus. 
as for LOAD_BACKGROUND, that allows the load to happen without triggering the
throbber (like onhover image loads, and such).  a LOAD_BACKGROUND request will
still be canceled when a new toplevel document is loaded.

Comment 9

[Darin Fisher]

Comment on attachment 118821 [details] [diff] [review]
Fix

>-  else
>-    NS_NOTREACHED("parser needs a content type to find a dtd");

maybe it would be better to trap the necko error earlier on.  or at least
explain in the code why it is ok to have no content-type.

sr=darin

Comment 10

[harishd]

Comment on attachment 118821 [details] [diff] [review]
Fix

r=harishd

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fixed.