718936 - firefox freeze while loading page with a large number of file:// links

Status: RESOLVED INCOMPLETE

(Core :: General, defect)

Description

[Shaohua Wen]

Firefox 9 is frozen while loading attached html page with a big table.

Tested with Fx 3.6 in Linux, works fine.

Comment 1

[Alice0775 White]

I can reproduce on
http://hg.mozilla.org/mozilla-central/rev/f4049f65efc6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120118 Firefox/12.0a1 ID:20120118031059

But not on
http://hg.mozilla.org/mozilla-central/rev/f4049f65efc6
Mozilla/5.0 (X11; Linux i686; rv:12.0a1) Gecko/20120118 Firefox/12.0a1 ID:20120118031059

Regression window
Works:
http://hg.mozilla.org/mozilla-central/rev/ad9729975289
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100224 Minefield/3.7a2pre ID:20100224075828
Hangs up for a while:
http://hg.mozilla.org/mozilla-central/rev/639b98ae11a8
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100224 Minefield/3.7a2pre ID:20100224094223
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ad9729975289&tochange=639b98ae11a8

In local build,
First bad : Shawn Wilsher — Bug 461199 (Part 13) - mozilla::dom::Link::SetLinkState should inform the document 
                                      about changes to its state when it is called. Make Link::SetLinkState notify the document
                                      about changes in state, plus a whole bunch of assertions for sanity checking. r=sicking r=bz
Last good : Shawn Wilsher — Bug 461199 (Part 12) - mozilla::dom::Link should unregister with mozilla::IHistory 
                                       when it goes away Call UnregisterWithHistory in Link's destructor to ensure that we are no 
                                       longer registered with IHistory. r=sicking sr=bz

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  I must be missing something...  When I load the page involved in either Firefox 9 or current trunk on Mac, I see no freeze.  Is this Windows-only?  Reporter, do you see this on Linux with a current build?

Comment 3

[Shaohua Wen]

Check nightly build 12.0a1 (2012-01-18) on linux i686, no freeze.

Comment 4

[Alice0775 White]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Hmm.  I must be missing something...  

FYI,
No hang up,if I commented out Link::ResetLinkState(false); in content/html/content/src/nsHTMLAnchorElement.cpp.
(Of course, though the state of the link will not function ...)

nsHTMLAnchorElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
                                nsIContent* aBindingParent,
                                bool aCompileEventHandlers)
{
  //Link::ResetLinkState(false);

Comment 5

[Marco Bonardo [:mak]]

this should likely be profiled.
fwiw, I noticed sort of a hang on Win7 on the first load, next reloads didn't hang.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK.  So the hang is definitely Windows-only?

But the code that changed is cross-platform... what's special about Windows here?  Is the page browser-sniffing?

Comment 7

[Alice0775 White]

Attachment: WinDbg log

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, interesting.  Is the freeze only visible when loading the page from a file:// URI, then?

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.  What does the windbg sample from that freeze look like?

Comment 10

[Alice0775 White]

Attachment: WinDbg log ( in case of open link of Comment#6 )

Comment 11

[Alice0775 White]

Er;
s/WinDbg log ( in case of open link of Comment#6 )/WinDbg log ( in case of open link of Comment#9 )/

Comment 12

[Boris Zbarsky [:bzbarsky]]

That's.... very confusing.   That last log is still showing you under nsLocalFile::Equals

Can you maybe take a look at what the file path for that nsLocalFile (or the mSpec for the corresponding nsStandardURL) is?

Comment 13

[Marco Bonardo [:mak]]

this may explain why further reloads are faster? I assume files stats are cached. Having to go through GetFileAttributesW for each url comparison is a pain :(

Comment 14

[Marco Bonardo [:mak]]

FWIW I just found bug 394486

Comment 15

[Marco Bonardo [:mak]]

I wonder if we may rather check for ~ in both urls to detect a short name and, if none of them has one, just do a case insensitive comparison. Otherwise fallback to the current code.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Oh, I'd missed the file:// URIs in the testcase.  That explains things.  I bet it's not an issue on other OSes because nsLocalFile::Equals is just an strcmp on the two file paths there.

Would it make sense to have the Windows nsLocalFile::Equals first compare mWorkingPath values and only if those differ do the canonical path thing?  That should help this case, because we're doing a hashtable lookup, so chances are the paths are equal anyway if we're comparing them to start with...

Comment 17

[Marco Bonardo [:mak]]

I fear that we'd hit far more difference than equality, especially when walking the hash. I suppose in most cases a page has all different file links.

I suppose we have to normalize because a working path may be in the long format and one in the short one, but both may point to the same file?
If so, we may use the fact short paths are always uppercase, so if mWorkingPath!=UC(mWorkingParh) for both files (none of them is a shortcut), there is no point in normalizing.

Comment 18

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #18)
> Would it make sense to have the Windows nsLocalFile::Equals first compare
> mWorkingPath values and only if those differ do the canonical path thing? 
> That should help this case, because we're doing a hashtable lookup, so
> chances are the paths are equal anyway if we're comparing them to start
> with...

Absolutely!

Comment 19

[Brian R. Bondy [:bbondy]]

Unmarking myself until I start to work on it. I tried to reproduce it but didn't have success, if someone else wants it in the meantime feel free.

Comment 20

[:glob ✱]

The content of attachment 589424 has been deleted by
    Byron Jones ‹:glob› <[e-mail]>
who provided the following reason:

Requested by creator in bug 735595

The token used to delete this attachment was generated at 2012-03-14 02:01:43 PDT.

Comment 21

[Liviu Cirdei]

Closing as incomplete because the testcase no longer exists. Fell free to reopen if you can still reproduce the issue and add a new testcase/testurl.