###!!! ASSERTION: op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0: 'op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0', file xpcom/build/pldhash.cpp, line 613

This bug worries me, in part because I don't like my fuzzer ignoring this generic assertion.

Hrm. I thought I'd commented on this bug... We should probably collect up all the external resources and then flush them all. dholbert, do you want to take this?

Sure. Might take a week or so to get to it, though.

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#6350 As bz said, we'll want to enumerate the resources with a callback that collects all of them into an nsTArray (I assume), then call Flush on the elements of that stable array.

Unassigning, as I haven't been able to get to this yet, and jdm is interested in mentoring a new contributor on this bug.

jdm, trying to get somewhere in with this bug, but im pretty lost. Have been having trouble with getting the testcode to work. could i get some help please? havent had much luck on irc.

Made changes requested and suggested by jdm and jaws. Thanks for your help!

This will give you a signed/unsigned mismatch on some platforms... > + int32_t n = resources.Length(); > + > + for(uint32_t i = 0; i < n; i++) You just need to make n and i the same type (probably int32_t if that's type of the return value of resources.Length()

Comment on attachment 707670 [details] [diff] [review] Fix for bug 705961 Review of attachment 707670 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks really close. Please make the following change as well as switching n to be a uint32_t since that is what nsTArray.Length() returns. ::: content/base/src/nsDocument.cpp @@ +6794,1 @@ > Flush(nsIDocument* aDocument, void* aData) The Flush function can be removed now since there are no other callers for it.

Comment on attachment 707670 [details] [diff] [review] Fix for bug 705961 Review of attachment 707670 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Bill! I've got some stylistic changes, but the functional change looks good to me. If you could attach a version with the requested changes, then I'll make sure it gets reviewed by someone who owns this code. ::: content/base/src/nsDocument.cpp @@ +6806,5 @@ > if (GetDisplayDocument()) { > return; > } > + nsTArray<nsIDocument*> resources; > + nit: don't need this newline @@ +6809,5 @@ > + nsTArray<nsIDocument*> resources; > + > + EnumerateExternalResources(Copy, &resources); > + > + int32_t n = resources.Length(); There shouldn't be any performance concerns with just calling this as part of the loop, so we don't need this local variable. @@ +6811,5 @@ > + EnumerateExternalResources(Copy, &resources); > + > + int32_t n = resources.Length(); > + > + for(uint32_t i = 0; i < n; i++) nit: add a space before the (, and move the { onto this line @@ +6814,5 @@ > + > + for(uint32_t i = 0; i < n; i++) > + { > + nsIDocument* doc = resources.ElementAt(i); > + doc->FlushPendingNotifications(aType); You can just use resources[i]->FlushPendingNotifications

Also, if you can follow the instructions at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F to generate a patch with your author information and a good commit message, that would be helpful.

Prevent ExternalResource hash from being modified during enumeration by creating temporary array to store external resources. After resources have been copy flush the array.

Looks like this still could benefit from patch headers (your name / email address, so that the patch is attributed to you, and a commit message) -- see the link in comment 13. Also, this isn't quite ready for checkin yet -- technically it needs "review+". The "feedback" flag (which jaws toggled to "+") is for informal suggestions etc., but it needs to have been granted review+ before it can land. So: I'd suggest following the steps in comment 13, so that those lines end up in your patch, and then post one final time and request review from someone who owns this area of code. (:bz would probably be appropriate.)

Comment on attachment 707747 [details] [diff] [review] Patch for Bug 705961: Addressed Review Comments Make it an nsTArray< nsCOMPtr<nsIDocument> > to be safe, and looks good. But this still needs commit metadata! r=me assuming all that gets fixed.

Prevent ExternalResource hash from being modified during enumeration by creating temporary array to store external resources. After resources have been copied, flush the array.

Bill, can you follow the steps from comment 13 to provide author information and a commit message as part of the patch? It will make it much easier for someone else to commit it once those are present.

In the interest of this patch not being forgotten, I went ahead and committed it with the following commit message: " Bug 705961 - Ensure nsDocument doesn't modify the external resource hashtable while enumerating it. r=bz" https://hg.mozilla.org/integration/mozilla-inbound/rev/d0efae27e013