Copy Page::ordinaryPages() before undeferring loads.

Copy Page::ordinaryPages() before undeferring loads.

Undeferring loads can run script, which can mutate the PageSet.

BUG=628942
Committed: https://crrev.com/5544c19b4252cbf65eea6894d55b2ed486957079
Cr-Commit-Position: refs/heads/master@{#412104}

[dcheng, 2016-08-13 02:30:00]

Description was changed from

==========
Copy Page::ordinaryPages() before undeferring loads.

Undeferring loads can run script, which can mutate the PageSet.

BUG=633182
==========

to

==========
Copy Page::ordinaryPages() before undeferring loads.

Undeferring loads can run script, which can mutate the PageSet.

BUG=628942
==========

[dcheng, 2016-08-15 21:57:27]

dcheng@chromium.org changed reviewers:
+ haraken@chromium.org

[dcheng, 2016-08-15 21:57:27]

I did attempt to write a test for this, but nothing I've written is particularly
satisfactory.

1) It's not easy to write a layout test for this, since triggering it in just JS
is not possible through content_shell only.

2) We could land the test case in the bug as-is, but we're actively trying to
remove the edge case that bug depends on.

3) I tried writing a unit test, and I haven't figured out a way to make it work
without history navigations. But once I add in history navigations, the unit
test becomes very complicated and I have to hardcode a bunch of
embedder-specific behavior: this means the unit test can easily end up diverging
from real behavior.

In the short-term, it seems more important to fix the bug so we can merge the
fix. In the meanwhile, I'll keep hacking on the unit test and see if I can get
it to a better place. WDYT?

https://codereview.chromium.org/2242923002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp (right):

https://codereview.chromium.org/2242923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp:40:
HeapVector<Member<Page>> pages;
I'm not sure if it's more standard for this to be Member (since that matches
what the old RefPtr semantics would have been: this collection strongly keeps it
live) or WeakMember (to match the type of PageSet). In theory, WeakMember here
is OK: I don't think Oilpan GC can collect in the middle here, but it's seems a
bit harder to understand.

So I ended up going with Member.

[haraken, 2016-08-15 22:01:47]

LGTM

https://codereview.chromium.org/2242923002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp (right):

https://codereview.chromium.org/2242923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp:40:
HeapVector<Member<Page>> pages;
On 2016/08/15 21:57:27, dcheng wrote:
> I'm not sure if it's more standard for this to be Member (since that matches
> what the old RefPtr semantics would have been: this collection strongly keeps
it
> live) or WeakMember (to match the type of PageSet). In theory, WeakMember here
> is OK: I don't think Oilpan GC can collect in the middle here, but it's seems
a
> bit harder to understand.
> 
> So I ended up going with Member.

HeapVector<Member<>> makes sense. We don't want to use WeakMember unless there's
any reason to do so (because of the overhead).

https://codereview.chromium.org/2242923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp:42:
pages.append(page);

Shall we add a comment and mention why we're copying?

[dcheng, 2016-08-15 22:34:28]

https://codereview.chromium.org/2242923002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp (right):

https://codereview.chromium.org/2242923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp:42:
pages.append(page);
On 2016/08/15 22:01:47, haraken wrote:
> 
> Shall we add a comment and mention why we're copying?

Done.

[dcheng, 2016-08-15 22:34:32]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-08-15 22:34:32]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2242923002/#ps20001
(title: "Add comment")

[commit-bot: I haz the power, 2016-08-15 22:34:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 23:58:04]

Description was changed from

==========
Copy Page::ordinaryPages() before undeferring loads.

Undeferring loads can run script, which can mutate the PageSet.

BUG=628942
==========

to

==========
Copy Page::ordinaryPages() before undeferring loads.

Undeferring loads can run script, which can mutate the PageSet.

BUG=628942
==========

[commit-bot: I haz the power, 2016-08-15 23:58:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-16 00:01:02]

Description was changed from

==========
Copy Page::ordinaryPages() before undeferring loads.

Undeferring loads can run script, which can mutate the PageSet.

BUG=628942
==========

to

==========
Copy Page::ordinaryPages() before undeferring loads.

Undeferring loads can run script, which can mutate the PageSet.

BUG=628942

Committed: https://crrev.com/5544c19b4252cbf65eea6894d55b2ed486957079
Cr-Commit-Position: refs/heads/master@{#412104}
==========

[commit-bot: I haz the power, 2016-08-16 00:01:03]

Patchset 2 (id:??) landed as
https://crrev.com/5544c19b4252cbf65eea6894d55b2ed486957079
Cr-Commit-Position: refs/heads/master@{#412104}

[esprehn, 2016-08-16 02:31:41]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-08-16 02:31:42]

https://codereview.chromium.org/2242923002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp (right):

https://codereview.chromium.org/2242923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp:44:
pages.append(page);
copyToVector(pages, Page::ordinaryPages())