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

RESOLVED FIXED in mozilla21

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: spartanfire)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla21
x86_64
Mac OS X
assertion, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=jdm][lang=c++][fuzzblocker:RECURSION_LEVEL])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 577446 [details]
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
(Reporter)

Comment 1

6 years ago
Created attachment 577447 [details]
stack trace
(Reporter)

Comment 2

6 years ago
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.
Assignee: nobody → dholbert+bmo
Status: NEW → ASSIGNED

Comment 5

6 years ago
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.
Whiteboard: [mentor=jdm][lang=c++]
Unassigning, as I haven't been able to get to this yet, and jdm is interested in mentoring a new contributor on this bug.
Assignee: dholbert → nobody
Status: ASSIGNED → NEW
(Reporter)

Updated

5 years ago
Whiteboard: [mentor=jdm][lang=c++] → [mentor=jdm][lang=c++][fuzzblocker:RECURSION_LEVEL]

Comment 7

5 years ago
http://hg.mozilla.org/mozilla-central/annotate/43d65f5d22b2/content/base/src/nsDocument.cpp#l6801
Assignee: nobody → guimdearaujo
Status: NEW → ASSIGNED
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
Created attachment 707670 [details] [diff] [review]
Fix for bug 705961

Made changes requested and suggested by jdm and jaws. Thanks for your help!
Attachment #707670 - Flags: review?(josh)
Attachment #707670 - Flags: review?(jaws)

Comment 10

5 years ago
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.
Attachment #707670 - Flags: review?(josh)
Attachment #707670 - Flags: review?(jaws)
Attachment #707670 - Flags: feedback+
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.
(Assignee)

Comment 14

5 years ago
Created attachment 707747 [details] [diff] [review]
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.
Attachment #707670 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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.)
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Attachment #707747 - Flags: review?(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.
Attachment #707747 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 709275 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/d0efae27e013
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.