642551 - FindUnfinishedRequestCallback is silly

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

I need to understand what the hell it's doing and why, but it's doing it in a dumb way that means that each load ending is O(N) in number of loads in the hashtable if they all have no status set!

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, this one comes up in test3 on bug 638235

Comment 2

[[e-mail]]

I don't suppose you can instrument it to find out which request it's finding? Otherwise, as a blind guess, I think nsDocLoader::OnStatus should cache the last request, and nsDocLoader::doStopURLLoad would only search the hash and update the status if it's the one that's just stopped. (In particular if the cache is null this means that there is no unfinished request.)

But what's supposed to happen if there are unfinished requests in a frame? Presumably they would have their own hash of requests which we'd overlook.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I don't suppose you can instrument it to find out which request it's finding?

I sure could.

But like I said, I'd like to understand what the heck this is doing to start with.  Presumably trying to not display stale status or something?

Comment 4

[[e-mail]]

(In reply to comment #3)
> But like I said, I'd like to understand what the heck this is doing to start
> with.  Presumably trying to not display stale status or something?
That was my guess too.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, in the case I was looking at this method is not finding a request at all.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Save status info we may want to use in a list so we don't have to walk the entire pending request hashtable looking for one of the rare ones with status info.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, I suppose I could avoid the separate allocation by putting the hashtable entries themselves into the list and using a nontrivial moveEntry callback.  But that would be a lot more complicated and error-prone, I think.

Comment 8

[[e-mail]]

Comment on attachment 554969 [details] [diff] [review]
Save status info we may want to use in a list so we don't have to walk the entire pending request hashtable looking for one of the rare ones with status info.

>+    if (info->mLastStatus) {
>+      // Null it out now so we don't find it when looking for status
>+      // from now on.  This destroys the nsStatusInfo and hence
>+      // removes it from our list.
>+      info->mLastStatus = nsnull;
>+    }
[Not sure that you need to bother with the null-check.]

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yeah, I'll nix the nullcheck.

Comment 10

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/de06684dabc4
http://hg.mozilla.org/integration/mozilla-inbound/rev/fc945dec50bb

Comment 11

[Phil Ringnalda (:philor)]

Backed out - https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=d01a282b5a40 should be the clearest rev to see the orange from this set, free from the leak that backout fixed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

This patch wasn't the problem; repushed as http://hg.mozilla.org/integration/mozilla-inbound/rev/003fedc37ecf

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/003fedc37ecf