FindUnfinishedRequestCallback is silly

RESOLVED FIXED in mozilla9

Status

()

Core
Document Navigation
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla9
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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!
Blocks: 638235
Oh, this one comes up in test3 on bug 638235

Comment 2

6 years ago
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.
> 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

6 years ago
(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.
Blocks: 680786
Oh, in the case I was looking at this method is not finding a request at all.
Created 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.
Whiteboard: [need review]
Attachment #554969 - Flags: review?(neil)
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

6 years ago
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.]
Attachment #554969 - Flags: review?(neil) → review+
Yeah, I'll nix the nullcheck.
Whiteboard: [need review] → [need landing]
http://hg.mozilla.org/integration/mozilla-inbound/rev/de06684dabc4
http://hg.mozilla.org/integration/mozilla-inbound/rev/fc945dec50bb
Flags: in-testsuite?
Whiteboard: [need landing]
Target Milestone: --- → mozilla9
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.
This patch wasn't the problem; repushed as http://hg.mozilla.org/integration/mozilla-inbound/rev/003fedc37ecf
http://hg.mozilla.org/mozilla-central/rev/003fedc37ecf
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.