Closed Bug 642551 Opened 9 years ago Closed 8 years ago

FindUnfinishedRequestCallback is silly

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
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?
(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.
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 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]
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.
http://hg.mozilla.org/mozilla-central/rev/003fedc37ecf
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.