[@import scanner] Don't access Document::settings() from appendData callback

[@import scanner] Don't access Document::settings() from appendData callback

This is a tentative fix for a common canary crasher.

BUG=645331
Committed: https://crrev.com/7fc99f412a0a5d358c496f73e7e5a9e7e64ef9da
Cr-Commit-Position: refs/heads/master@{#417841}

[Charlie Harrison, 2016-09-09 02:50:08]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 02:50:31]

Dry run: 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-09-09 04:11:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 04:11:23]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-09-09 04:40:33]

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

[Charlie Harrison, 2016-09-09 04:40:34]

dcheng, would you mind taking a look? Are there cases when a
Document::settings() would be null when previously it wasn't? Can the frame
detach when the document still lives?

I'd like to write a test for this but I'm not exactly sure what is going on in
the crash stack traces.

[dcheng, 2016-09-10 02:42:50]

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

[dcheng, 2016-09-10 02:42:50]

Yeah, it's definitely possible: one easy way is for a Document to be detached
while there's still a DOM reference to it, e.g. something like this:

var iframe = document.createElement('iframe');
document.body.appendChild(iframe);
var frameDocument = iframe.contentDocument;
iframe.parentNode.removeChild(iframe);
// Frame's document is still alive from JS reference.

That being said, I'm a bit surprised that we're hitting this crash: we
specifically try to stop all the loaders when we detach a frame. So I'm not sure
how this is happening... that being said, I'm not that familiar with how
preloading and the parser tie into Document lifetimes.  +kouhei, any thoughts?

[kouhei (in TOK), 2016-09-10 16:50:52]

change lgtm

> That being said, I'm a bit surprised that we're hitting this crash: we
> specifically try to stop all the loaders when we detach a frame. So I'm not
sure
> how this is happening... that being said, I'm not that familiar with how
> preloading and the parser tie into Document lifetimes.  +kouhei, any thoughts?
I've also seen in other places where the code expects Resource callbacks to
outlive its attached Document, so I think this is the current design.

[Charlie Harrison, 2016-09-10 17:06:47]

Thanks. Because this is a common crasher I'll land this now with follow up
tests.

[Charlie Harrison, 2016-09-10 17:18:13]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-09-10 17:18:20]

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-09-10 18:44:52]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-10 18:47:49]

Description was changed from

==========
[@import scanner] Don't access Document::settings() from appendData callback

This is a tentative fix for a common canary crasher.

BUG=645331
==========

to

==========
[@import scanner] Don't access Document::settings() from appendData callback

This is a tentative fix for a common canary crasher.

BUG=645331

Committed: https://crrev.com/7fc99f412a0a5d358c496f73e7e5a9e7e64ef9da
Cr-Commit-Position: refs/heads/master@{#417841}
==========

[commit-bot: I haz the power, 2016-09-10 18:47:50]

Patchset 1 (id:??) landed as
https://crrev.com/7fc99f412a0a5d358c496f73e7e5a9e7e64ef9da
Cr-Commit-Position: refs/heads/master@{#417841}