99666 - page is displayed as blank in certain situation

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Yuying Long]

Build: 09-14 0.9.4 Branch build

Steps to reproduce:
1. Launch browser.
2. View | Character Coding | Auto-detect

3. -- Windows: "Universal" is there 
   -- Mac9.1-Ja and Linux RH6.2-Ja: "Universal" is not there

4. Go the page: http://home.netscape.com
   -- Windows: set Auto-Detect to "Universal" or "All"
   -- Mac and Linux: set Auto-Detect "All"

5. Click on the top news "Suspects Named" of 09-14 - I'll attach this page later.

6. A blank page with status "Done" in status bar will be loaded, if you reload
the page, then the correct page gets load correctly.

Note: Clear cache will help you to reproduce the problem.

Comment 1

[Yuying Long]

Attachment: Page refer to link "Suspects Named" of Netscape homepage

Comment 2

[Yuying Long]

Attachment: In case, 09-14 Netscape home page

Comment 3

[msanz]

Yuying, is this a regression from 6.1?

Comment 4

[Yuying Long]

Yes.  We don't have Universal auto-detect before, and auto-detect All was work
fine before.

I believe it's cause by the check-in of Universal detector.

Comment 5

[Jaime Rodriguez, Jr.]

This sounds more like a problem with a new feature, then a regression to me.
Will the fix be ready soon? What is the user impact of this bug.

Comment 6

[ji]

Added the URL which can be used to reproduce the problem. It only happens to
some web pages. http://cncenter.sina.com/newsEvent/index.html is also one of them.
Shanjian, could it be the case again that the auto-detector itself has no
problem, but just a trigger to the problem we saw?

Comment 7

[msanz]

is it a new feature? Yuying, did you mean that it is new or that the problem
wasn't there before?

Comment 8

[Yuying Long]

1. It's a new feature for auto-detect "Universal", however, the problems for
this are:
a. it's on Windows but not on Mac and Linux.
b. doens't work well on Windows.

2. And for the problem with auto-detect All not work well is a regression.

Comment 9

[msanz]

adding nsbranch+ and regression keyword for the auto-detect all part

Comment 10

[Shanjian Li]

I reproduced the problem using  http://cncenter.sina.com/newsEvent/index.html. 
Page like this one and also netscape's home page all have metacharset specified. 
Especially this one, the meta charset is specified in the beginning. So charset 
sniffing should work. Tracing inside universal charset detector shows the 
detector does got called and report something, but the reported charset is fine 
and browser should at least show something. 

My guess is something got recent checked in caused all this problem. The problem 
is most likely caused by certain timing factor, which universal charset detector 
play a role. This is a serious regression and should be resolved. I will 
continue to look into this. In the meantime, please let parser and cache people 
got involved. 

Comment 11

[msanz]

Shanjian, who should we involve? Add the names to the cc list

Comment 12

[Shanjian Li]

add harish, he knows well about parser

Comment 13

[Shanjian Li]

add gagan, who knows cache issues. 

Comment 14

[Gagan]

that would really be gordon :)

Comment 15

[Shanjian Li]

yuying, could you help find out when did we begin to have this problem?  This 
may help figure our whose checkin cause the problem. If my guess is correct (a 
racing problem), this bug might be a little bit hard to trace. 

Comment 16

[Shanjian Li]

harish, I found something very interesting. I am not sure if that can provide 
some hint to the problem. In nsParser, 

nsresult nsParser::OnStopRequest(nsIRequest *request, nsISupports* aContext,
                                 nsresult status)
{  

  nsresult result=NS_OK;
  
  if(eOnStart==mParserContext->mStreamListenerState) {

    //If you're here, then OnDataAvailable() never got called. 
    //Prior to necko, we never dealt with this case, but the problem may have 
existed.
    //What we'll do (for now at least) is construct a blank HTML document.
    nsAutoString  temp; temp.AssignWithConversion("<html><body></body></html>");
    mParserContext->mScanner->Append(temp);
    result=ResumeParse(PR_TRUE,PR_TRUE);    
  }

The first if is true and a empty document is constructed. That might be the 
blank page we are observing. Do you have any idea why this function got called 
without any data sent through OnDataAvailable()?

Comment 17

[harishd]

I belive that the code ( added by rickg ) is there just to ensure that the
browser window gets painted when something bad, such as OnStartRequest getting
fired before OnDataAvailable, happens. If we hit that condition then we need to
trace back and see who is responsible for not firing OnDataAvailable.

Comment 18

[Yuying Long]

Auto-detect All regression is some time between 08-27 ~ 09-03.

Comment 19

[Shanjian Li]

Attachment: A simple testcase for this

Comment 20

[Shanjian Li]

I simplified a problem html page from a sina website. There is nothing special 
for this page except it contains some special multi-byte characters which make 
the universal charset detector report something. When the detector report 
something, it confuse the browser in certain situation. 

Using shift-reload, you should see this problem on and off consistently. 

Comment 21

[Shanjian Li]

the summary and component of this bug need to be changed. (I just found out that 
I did not have the privilege to do so.)

Cache is just a primary suspect, the problem might turn out to be somewhere 
else, like parser, layout. But I am quite confident that it is not detector's 
problem, though the detector act like a trigger. (According to yuying, this 
regression began to happen in 8/27 to 9/3, and the last checkin for charset 
detector in commercial tree was on 8/1.) 

Please reassign to the right person. 

Comment 22

[Shanjian Li]

I changed the summary and component, and reassign this bug to gordon. As I 
stated earlier, the problem might turn out to be somewhere else. Cache is the 
top suspect in my mind. 

Comment 23

[Shanjian Li]

I spent more time on this, and I found that after charset autodetector 
notification, parser received data and model is built. So that seems 
network/cache is OK. I don't know why layout/refresh is not really initiated. 
It seems like some state is mismanipulated inside parser/viewer/layout. 
Reassign to harishd and hope he can provide more information. 

Comment 24

[Shanjian Li]

Harish, I just pinned down the checkin that cause this problem. It is related to 
your checkin to nsMetaCharsetObserver.cpp on 8/27. Backing out your patch in 
current branch can prevent this problem from happening. I am not quite clear at 
this time what is really happening, and your fix might be just a trigger of a 
old problem. I will continue to look into this problem in one or 2 hours. You 
probably have better understanding what the problem is. So I will keep this 
assigned to you and please help. 

Comment 25

[harishd]

hmm, it looks like the DTD's internal state ( NS_ERROR_HTMLPARSER_STOPPARSING
)did not get reset to ( NS_OK ) when reloading the document with new charset.
This would explain why the content model wasn't getting created. Will work on it.

Comment 26

[Shanjian Li]

Attachment: patch part1

Comment 27

[Shanjian Li]

Attachment: patch part2

Comment 28

[Shanjian Li]

Harish pointed to me where the problem is. I just posted a patch. I will get the 
bug back and try to fix it in emojo. 
Harish, could you review my fix? 

Comment 29

[Shanjian Li]

Attachment: revised version of part1, but it should only apply to trunk.

Comment 30

[Shanjian Li]

Harish suggested to default a specific error message. This has been done in the 
revised part1 patch. However, since NS_ERROR_MODULE_GENERAL is not available in 
branch yet, I would suggest to use part1 instead.  To be more clear, I want to :
. Apply patch part1 and part2 to branch. 
. part2 and revised part1  for trunk. 

Comment 31

[harishd]

Shanjian: Could you please run browser buster ( international sites ) with your
patch? Browser buster could be found in http://komodo.mozilla.org.

Comment 32

[Shanjian Li]

I am running the buster now. So far so good.

Comment 33

[Shanjian Li]

bobj: Will you please add a comment to characterize what conditions will cause 
this bug to occur?
  (From the comments, it sounds like this bug has nothing to do with autodetect 
and only with meta charset handling.)

When universal charset detector or charset detect all is on, and a webpage also 
has a meta charset specified, if meta charset observer send its notification 
after autodetector's notification, its request will be rejected by webshell 
because a charset reloading is already on its way. However, the error message 
returned from observer's notification function will stop parser from continuing 
parsing. Theoritically, this may happen with other detectors as well. Because 
universal charset detector is able to reach its conclusion earlier than other 
detectors, it is more likely to happen when universal charset detector is 
selected. So simply speaking, if a detector is selected and the webpage has 
meta charset specified, this problem will happen in certain percentage. (for 
universal charset detector, this percentage is simply too high. My guess is 
50%.)

Comment 34

[Shanjian Li]

harish, I have run both international and top 100 and did not encouner any 
problem. 

Comment 35

[harishd]

Comment on attachment 49808 [details] [diff] [review]
patch part1

r=harishd

Comment 36

[Shanjian Li]

chris, could you sr' my fix?

Comment 37

[Chris Waterson]

I think rpotts might have a better handle on this than I would for sr purposes.

Comment 38

[rpotts (gone)]

Comment on attachment 49808 [details] [diff] [review]
patch part1

I think this looks ok...

After these changes, we *really* should rename the nsIWebServices methods to better reflect that they *only* apply to charset-reload and are not generic...

Comment 39

[rpotts (gone)]

Comment on attachment 49810 [details] [diff] [review]
patch part2

sr=[e-mail]

Comment 40

[rpotts (gone)]

Comment on attachment 49808 [details] [diff] [review]
patch part1

oops... forgot sr=[e-mail]

Comment 41

[Jaime Rodriguez, Jr.]

shanjian - make sure to update the patch status on the final patch.

Comment 42

[Jaime Rodriguez, Jr.]

This looks like a nice little patch, with some upside. What kinda of testing
have we done on it? What are the risk, if we take it (i.e. What areas will we
break)? How easy is it to back out?

Comment 43

[Shanjian Li]

> What kinda of testing have we done on it? 
I did browser buster tested as suggested by harishd. top 100 site and top 100 
international site and some random sites are tested. No problem found.

>What are the risk, if we take it (i.e. What areas will we break)? 
I couldn't think of any yet. If our request of reload is rejected, we just tell 
parser that nothing happened. To parser that's true. Since nothing really 
happen, nothing should go wrong because of this. 

>How easy is it to back out?
For branch, there are 2 files involved, and the fix is not complicated. The back 
out process is just the same as anything else. 

Comment 44

[Shanjian Li]

fix checked into trunk. 

Comment 45

[Jaime Rodriguez, Jr.]

Shanjian/Ftang - Pls come to the PDT tomorrow @ noon to discuss the progress on
this bug.

Comment 46

[Jaime Rodriguez, Jr.]

check it in - PDT+

Comment 47

[Shanjian Li]

fix checked in to branch. 

Comment 48

[Yuying Long]

Fixed verified on both 09-24 and 09-25 branch build.

Comment 49

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 102365 has been marked as a duplicate of this bug. ***