Last Comment Bug 705961 - Under nsDocument::FlushExternalResources, the ExternalResource hash is modified during enumeration
: Under nsDocument::FlushExternalResources, the ExternalResource hash is modifi...
Status: RESOLVED FIXED
[mentor=jdm][lang=c++][fuzzblocker:RE...
: assertion, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla21
Assigned To: Bill de Araujo [:spartanfire]
:
Mentors:
Depends on:
Blocks: randomstyles
  Show dependency treegraph
 
Reported: 2011-11-28 17:45 PST by Jesse Ruderman
Modified: 2013-02-04 16:34 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (222 bytes, text/html)
2011-11-28 17:45 PST, Jesse Ruderman
no flags Details
stack trace (12.57 KB, text/plain)
2011-11-28 17:46 PST, Jesse Ruderman
no flags Details
Fix for bug 705961 (1.16 KB, patch)
2013-01-29 09:05 PST, Bill de Araujo [:spartanfire]
jaws: feedback+
Details | Diff | Review
Patch for Bug 705961: Addressed Review Comments (1.13 KB, patch)
2013-01-29 11:35 PST, Bill de Araujo [:spartanfire]
bzbarsky: review+
Details | Diff | Review
Patch for Bug 705961: Addressed Final Review Comments (1.46 KB, patch)
2013-02-01 16:09 PST, Bill de Araujo [:spartanfire]
no flags Details | Diff | Review

Description Jesse Ruderman 2011-11-28 17:45:28 PST
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
Comment 1 Jesse Ruderman 2011-11-28 17:46:24 PST
Created attachment 577447 [details]
stack trace
Comment 2 Jesse Ruderman 2011-12-05 02:19:32 PST
This bug worries me, in part because I don't like my fuzzer ignoring this generic assertion.
Comment 3 Boris Zbarsky [:bz] 2011-12-05 20:26:10 PST
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] 2011-12-07 16:38:00 PST
Sure.  Might take a week or so to get to it, though.
Comment 5 Josh Matthews [:jdm] 2012-02-28 01:01:09 PST
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] 2012-03-30 15:06:28 PDT
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 8 Bill de Araujo [:spartanfire] 2013-01-16 14:17:36 PST
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] 2013-01-29 09:05:14 PST
Created attachment 707670 [details] [diff] [review]
Fix for bug 705961

Made changes requested and suggested by jdm and jaws. Thanks for your help!
Comment 10 Robert Longson 2013-01-29 09:10:00 PST
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) 2013-01-29 09:23:48 PST
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] 2013-01-29 09:25:06 PST
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] 2013-01-29 09:26:54 PST
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] 2013-01-29 11:35:57 PST
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.
Comment 15 Daniel Holbert [:dholbert] 2013-01-29 11:46:51 PST
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 [:bz] 2013-01-31 10:48:36 PST
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] 2013-02-01 16:09:33 PST
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.
Comment 18 Josh Matthews [:jdm] 2013-02-01 16:18:14 PST
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] 2013-02-04 09:23:30 PST
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] 2013-02-04 16:34:35 PST
https://hg.mozilla.org/mozilla-central/rev/d0efae27e013

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