Fix UAF in chrome_pdf::Instance::GetURL()

Fix UAF in chrome_pdf::Instance::GetURL()

The instance owns the engine via its engine_ scoped_ptr, so if the engine is 
being destroyed via the scoped_ptr destructor, it may not be safe to access 
anything in the instance since the instance may be partially destroyed. Instead,
destroy the engine as the first step in the process so the instance is still
intact.

BUG=392956
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287955

[Tom Sepez, 2014-07-30 21:36:37]

John, please review or suggest an alternate.

[Tom Sepez, 2014-07-30 23:41:17]

On 2014/07/30 21:36:37, Tom Sepez wrote:
> John, please review or suggest an alternate.
PS #1 isnt't quite sufficient but maybe it should land first since PS #2 is
large.

[Tom Sepez, 2014-07-31 17:52:03]

The latter seems to knock out several other CF test cases.  I'm wondering if we
should move the logic in the engine destructor into a WillBeDestroyed() method. 
This might go cleaner.

[Tom Sepez, 2014-07-31 18:19:15]

It looks like WillBeDestroyed() called early on in the instance destructor is
too late.

[palmer, 2014-07-31 18:31:57]

lgtm

[jam, 2014-07-31 22:22:32]

https://codereview.chromium.org/427583003/diff/20001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/427583003/diff/20001/pdf/instance.cc#newcode310
pdf/instance.cc:310: engine_->ClientDestroyed();
why not just engine_.reset() here?

[Tom Sepez, 2014-08-01 16:08:20]

On 2014/07/31 22:22:32, jam wrote:
> https://codereview.chromium.org/427583003/diff/20001/pdf/instance.cc
> File pdf/instance.cc (right):
> 
>
https://codereview.chromium.org/427583003/diff/20001/pdf/instance.cc#newcode310
> pdf/instance.cc:310: engine_->ClientDestroyed();
> why not just engine_.reset() here?

Will do.

[Tom Sepez, 2014-08-06 20:45:35]

John, please re-review.  It looks like your suggestion takes care of the test
case by itself.

[jam, 2014-08-06 21:15:30]

On 2014/08/06 20:45:35, Tom Sepez wrote:
> John, please re-review.  It looks like your suggestion takes care of the test
> case by itself.

lgtm

nit: it would be nice to add a comment beside the reset explaining why it's done

[Tom Sepez, 2014-08-06 21:43:36]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2014-08-06 21:44:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tsepez@chromium.org/427583003/80001

[commit-bot: I haz the power, 2014-08-07 03:32:54]

Change committed as 287955