705961 - Under nsDocument::FlushExternalResources, the ExternalResource hash is modified during enumeration

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

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

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Jesse Ruderman]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Daniel Holbert [:dholbert]]

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

Comment 5

[Josh Matthews [:jdm]]

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.

Comment 6

[Daniel Holbert [:dholbert]]

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

Comment 7

[Josh Matthews [:jdm]]

http://hg.mozilla.org/mozilla-central/annotate/43d65f5d22b2/content/base/src/nsDocument.cpp#l6801

Comment 8

[Bill de Araujo [:spartanfire]]

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.

Comment 9

[Bill de Araujo [:spartanfire]]

Attachment: Fix for bug 705961

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

Comment 10

[Robert Longson [:longsonr]]

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 11

[Jared Wein [:jaws] (please needinfo? me)]

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 12

[Josh Matthews [:jdm]]

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

Comment 13

[Josh Matthews [:jdm]]

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.

Comment 14

[Bill de Araujo [:spartanfire]]

Attachment: Patch for Bug 705961: Addressed Review Comments

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

Comment 15

[Daniel Holbert [:dholbert]]

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 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Bill de Araujo [:spartanfire]]

Attachment: Patch for Bug 705961: Addressed Final Review Comments

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

Comment 18

[Josh Matthews [:jdm]]

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.

Comment 19

[Josh Matthews [:jdm]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0efae27e013