Fix some iterator invalidation issues while traversing CPDF_Dictionary.

Fix some iterator invalidation issues while traversing CPDF_Dictionary.

Also fixes a potential issue in CPDF_Dictionary::ReplaceKey.

R=thestig@chromium.org
BUG=577030
Committed: https://pdfium.googlesource.com/pdfium/+/cae57daaa0f7ed4c92e22c4e7ef30392393d1128

[Oliver Chang, 2016-01-14 00:34:24]

Description was changed from

==========
Fix some iterator invalidation issues with iterating CPDF_Dictionary.
==========

to

==========
Fix some iterator invalidation issues while iterating CPDF_Dictionary.
==========

[Oliver Chang, 2016-01-14 00:45:43]

Description was changed from

==========
Fix some iterator invalidation issues while iterating CPDF_Dictionary.
==========

to

==========
Fix some iterator invalidation issues while traversing CPDF_Dictionary.
==========

[Oliver Chang, 2016-01-14 00:46:06]

Patchset #2 (id:20001) has been deleted

[Oliver Chang, 2016-01-14 00:47:30]

Patchset #2 (id:40001) has been deleted

[Oliver Chang, 2016-01-14 00:48:54]

Description was changed from

==========
Fix some iterator invalidation issues while traversing CPDF_Dictionary.
==========

to

==========
Fix some iterator invalidation issues while traversing CPDF_Dictionary.

R=thestig@chromium.org
BUG=577030
==========

[Oliver Chang, 2016-01-14 00:48:54]

ochang@chromium.org changed reviewers:
+ thestig@chromium.org

[Oliver Chang, 2016-01-14 00:50:32]

ptal. also replaces fix in https://codereview.chromium.org/1581963004/

[Oliver Chang, 2016-01-14 00:54:26]

Patchset #3 (id:80001) has been deleted

[Oliver Chang, 2016-01-14 00:55:52]

Patchset #3 (id:100001) has been deleted

[Oliver Chang, 2016-01-14 00:56:30]

Description was changed from

==========
Fix some iterator invalidation issues while traversing CPDF_Dictionary.

R=thestig@chromium.org
BUG=577030
==========

to

==========
Fix some iterator invalidation issues while traversing CPDF_Dictionary.

Also fixes a potential issue in CPDF_Dictionary::ReplaceKey.

R=thestig@chromium.org
BUG=577030
==========

[Lei Zhang, 2016-01-14 01:32:30]

https://codereview.chromium.org/1582963003/diff/120001/core/include/fpdfapi/f...
File core/include/fpdfapi/fpdf_objects.h (right):

https://codereview.chromium.org/1582963003/diff/120001/core/include/fpdfapi/f...
core/include/fpdfapi/fpdf_objects.h:388: // Set* functions invalidates iterators
for the given key.
-> Set* functions invalidates iterators for |key|.

https://codereview.chromium.org/1582963003/diff/120001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1582963003/diff/120001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:737: if (new_it == old_it)
Can't we move this to the top and check if |oldkey| and |newkey| are equivalent?

https://codereview.chromium.org/1582963003/diff/120001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1582963003/diff/120001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:890: auto old_it = it++;
You don't need |old_it|. You can just extract |key| and |pElement|, and then
increment |it|.

[Oliver Chang, 2016-01-14 01:44:21]

https://codereview.chromium.org/1582963003/diff/120001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1582963003/diff/120001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:737: if (new_it == old_it)
On 2016/01/14 01:32:29, Lei Zhang wrote:
> Can't we move this to the top and check if |oldkey| and |newkey| are
equivalent?

Hmm, I did that at first, but thought that this was probably a bit more
efficient for the common case (keys are different)?

https://codereview.chromium.org/1582963003/diff/120001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1582963003/diff/120001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:890: auto old_it = it++;
On 2016/01/14 01:32:30, Lei Zhang wrote:
> You don't need |old_it|. You can just extract |key| and |pElement|, and then
> increment |it|.

Done.

[Lei Zhang, 2016-01-14 01:51:40]

lgtm, but you need to rebase and decide whether to undo my fix or not.

[Oliver Chang, 2016-01-14 02:04:53]

thanks, let's go with your fix

https://codereview.chromium.org/1582963003/diff/120001/core/include/fpdfapi/f...
File core/include/fpdfapi/fpdf_objects.h (right):

https://codereview.chromium.org/1582963003/diff/120001/core/include/fpdfapi/f...
core/include/fpdfapi/fpdf_objects.h:388: // Set* functions invalidates iterators
for the given key.
On 2016/01/14 01:32:29, Lei Zhang wrote:
> -> Set* functions invalidates iterators for |key|.

oops, I completely missed this comment, and made my own (similar) changes. do
they look OK?

[Lei Zhang, 2016-01-14 02:13:21]

On 2016/01/14 02:04:53, Oliver Chang wrote:
> thanks, let's go with your fix
> 
>
https://codereview.chromium.org/1582963003/diff/120001/core/include/fpdfapi/f...
> File core/include/fpdfapi/fpdf_objects.h (right):
> 
>
https://codereview.chromium.org/1582963003/diff/120001/core/include/fpdfapi/f...
> core/include/fpdfapi/fpdf_objects.h:388: // Set* functions invalidates
iterators
> for the given key.
> On 2016/01/14 01:32:29, Lei Zhang wrote:
> > -> Set* functions invalidates iterators for |key|.
> 
> oops, I completely missed this comment, and made my own (similar) changes. do
> they look OK?

Ya, it's fine.

[Oliver Chang, 2016-01-14 02:32:17]

Description was changed from

==========
Fix some iterator invalidation issues while traversing CPDF_Dictionary.

Also fixes a potential issue in CPDF_Dictionary::ReplaceKey.

R=thestig@chromium.org
BUG=577030
==========

to

==========
Fix some iterator invalidation issues while traversing CPDF_Dictionary.

Also fixes a potential issue in CPDF_Dictionary::ReplaceKey.

R=thestig@chromium.org
BUG=577030

Committed:
https://pdfium.googlesource.com/pdfium/+/cae57daaa0f7ed4c92e22c4e7ef30392393d...
==========

[Oliver Chang, 2016-01-14 02:32:18]

Committed patchset #5 (id:160001) manually as
cae57daaa0f7ed4c92e22c4e7ef30392393d1128 (presubmit successful).