Last Comment Bug 642551 - FindUnfinishedRequestCallback is silly
: FindUnfinishedRequestCallback is silly
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: x86 Mac OS X
: P1 normal (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 638235 680786
  Show dependency treegraph
 
Reported: 2011-03-17 12:26 PDT by Boris Zbarsky [:bz]
Modified: 2011-09-08 10:22 PDT (History)
9 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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. (8.05 KB, patch)
2011-08-22 14:43 PDT, Boris Zbarsky [:bz]
neil: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2011-03-17 12:26:08 PDT
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 [:bz] 2011-03-17 12:40:29 PDT
Oh, this one comes up in test3 on bug 638235
Comment 2 neil@parkwaycc.co.uk 2011-03-17 13:00:34 PDT
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 [:bz] 2011-03-17 13:05:15 PDT
> 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 neil@parkwaycc.co.uk 2011-03-17 13:36:14 PDT
(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 [:bz] 2011-08-22 14:42:54 PDT
Oh, in the case I was looking at this method is not finding a request at all.
Comment 6 Boris Zbarsky [:bz] 2011-08-22 14:43:34 PDT
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.
Comment 7 Boris Zbarsky [:bz] 2011-08-23 09:21:29 PDT
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 neil@parkwaycc.co.uk 2011-09-04 02:56:56 PDT
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 [:bz] 2011-09-06 19:55:47 PDT
Yeah, I'll nix the nullcheck.
Comment 11 Phil Ringnalda (:philor, back in August) 2011-09-06 22:12:48 PDT
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 [:bz] 2011-09-07 07:02:32 PDT
This patch wasn't the problem; repushed as http://hg.mozilla.org/integration/mozilla-inbound/rev/003fedc37ecf

Note You need to log in before you can comment on or make changes to this bug.