Invalidate the previous frame when the window object is cleared.

Invalidate the previous frame when the window object is cleared.

When a javascript:-URLs is loaded and returns a non-void value, then
Blink replaces the previously loaded document with the result, which
replaces the existing document and view. These events do not trigger
any document load notifications, which resulted in an invalid frame->
content script mapping.

This patch resolves this problem by invalidating the previous frame
when the document is created instead of at provisional load). This
patch depends on https://codereview.chromium.org/710443002.

BUG=416907
R=kalman@chromium.org

Committed: https://crrev.com/5ef11ff1b68256eb46ce90f5f14a7bd8cacb51a8
Cr-Commit-Position: refs/heads/master@{#304522}

[not at google - send to devlin, 2014-10-29 17:36:04]

https://codereview.chromium.org/684143002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/manifest.json
(right):

https://codereview.chromium.org/684143002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/manifest.json:34:
"js": ["end.js"],
Am I missing something? where is start.js and end.js? I'm confused as to how
this test exercises the bug you've fixed, maybe that's why.

https://codereview.chromium.org/684143002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js
(right):

https://codereview.chromium.org/684143002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js:5:
var expectMessageFromChild = false;
These global variables controlling test behaviour is pretty hard to follow. It
could be nice to use chrome.test.listenOnce/Forever with a helper function.
Perhaps:

var onRequest = chrome.extension.onRequest;

function checkRequestFrom(who) {
  return function(request) {
    chrome.test.assertEqual(who, request);
    chrome.test.succeed();
  };
}

function testDontInjectInAboutBlankFrame() {
  ...
  chrome.test.listenOnce(onRequest, checkRequestFrom('parent'));
},
function testDontInjectInAboutSrcdocFrame() {
  ...
  chrome.test.listenOnce(onRequest, checkRequestFrom('parent'));
},
...
function testDocumentStartRunsInSameWorldAsDocumentEnd() {
  var done = chrome.test.listenForever(onRequest, function(request) {
    if (request == 'parent')
      return;
    checkRequestFrom('child')();
    done();
  });
}

if that's what you're going for.

[robwu, 2014-10-29 18:43:12]

https://codereview.chromium.org/684143002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/manifest.json
(right):

https://codereview.chromium.org/684143002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/manifest.json:34:
"js": ["end.js"],
On 2014/10/29 17:36:04, kalman wrote:
> Am I missing something? where is start.js and end.js? I'm confused as to how
> this test exercises the bug you've fixed, maybe that's why.

Added missing files, I forgot to check them in.

https://codereview.chromium.org/684143002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js
(right):

https://codereview.chromium.org/684143002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js:5:
var expectMessageFromChild = false;
On 2014/10/29 17:36:04, kalman wrote:
> These global variables controlling test behaviour is pretty hard to follow. It
> could be nice to use chrome.test.listenOnce/Forever with a helper function.
> Perhaps:
> 
> var onRequest = chrome.extension.onRequest;
> 
> function checkRequestFrom(who) {
>   return function(request) {
>     chrome.test.assertEqual(who, request);
>     chrome.test.succeed();

Thanks for the suggestion. It turns out that listenOnce automatically calls
chrome.test.succeed()... ...and that incorrect state is not detected by tests:
https://crbug.com/428416

>   };
> }
> 
> function testDontInjectInAboutBlankFrame() {
>   ...
>   chrome.test.listenOnce(onRequest, checkRequestFrom('parent'));
> },
> function testDontInjectInAboutSrcdocFrame() {
>   ...
>   chrome.test.listenOnce(onRequest, checkRequestFrom('parent'));
> },
> ...
> function testDocumentStartRunsInSameWorldAsDocumentEnd() {
>   var done = chrome.test.listenForever(onRequest, function(request) {
>     if (request == 'parent')
>       return;
>     checkRequestFrom('child')();
>     done();
>   });
> }
> 
> if that's what you're going for.

Done.

[not at google - send to devlin, 2014-10-29 19:27:27]

Looks fine I guess, but does the test you added actually fail without the patch?
What part?

https://codereview.chromium.org/684143002/diff/20001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/end.js
(right):

https://codereview.chromium.org/684143002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/end.js:6:
if (typeof has_run_contentscript_at_document_start == 'undefined') {
This will crash unexpectedly unless you qualify
has_run_contentscript_at_document_start with window.has_blah.

Also use camel case in javascript.

https://codereview.chromium.org/684143002/diff/20001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js
(right):

https://codereview.chromium.org/684143002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js:50:
checkFirstMessageEquals('child')(request, true);
the 'true' seems redundant.

[robwu, 2014-10-29 21:05:12]

Patchset #3 (id:40001) has been deleted

[robwu, 2014-10-29 21:19:48]

On 2014/10/29 19:27:27, kalman wrote:
> Looks fine I guess, but does the test you added actually fail without the
patch?

Now it does.
Without patch, the last test fails with the following error:
"Unexpected request: "document_start script has not run!""

With this patch applied, the test passes (which indicates that the content
script correctly runs at document_start and document_end).

https://codereview.chromium.org/684143002/diff/20001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/end.js
(right):

https://codereview.chromium.org/684143002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/end.js:6:
if (typeof has_run_contentscript_at_document_start == 'undefined') {
On 2014/10/29 19:27:27, kalman wrote:
> This will crash unexpectedly unless you qualify
> has_run_contentscript_at_document_start with window.has_blah.

The typeof operator also works for undeclared variables.

> 
> Also use camel case in javascript.

Done.

https://codereview.chromium.org/684143002/diff/20001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js
(right):

https://codereview.chromium.org/684143002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/content_scripts/about_blank_iframes/test.js:50:
checkFirstMessageEquals('child')(request, true);
On 2014/10/29 19:27:27, kalman wrote:
> the 'true' seems redundant.

Done.

[not at google - send to devlin, 2014-10-29 21:22:25]

lgtm

[robwu, 2014-10-29 21:22:46]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-10-29 21:24:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684143002/60001

[commit-bot: I haz the power, 2014-10-29 22:50:13]

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

[commit-bot: I haz the power, 2014-10-29 22:50:13]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[robwu, 2014-10-30 00:14:09]

On 2014/10/29 22:50:13, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_rel on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Ok, bad idea to use dispatchDidClearDocumentOfWindowObject. It has too many
negative positives; in particular, when the main world is created, e.g. by
having a script on a page, by opening the devtools or by running a
javascript:-URL from the omnibox, this notification is also triggered.

I think that we have to use Blink's willReleaseScriptContext to reliably detect
that a frame is disposed. Unfortunately that notification is not exposed to
RenderViewObserver, so I have to find a better alternative (possibly by somehow
subscribing to RenderFrameObserver?)

[not at google - send to devlin, 2014-10-30 00:20:57]

Dealing with script contexts isn't right either. Are there not other methods on
RenderViewOberver which might help?

[robwu, 2014-10-30 00:34:58]

On 2014/10/30 00:20:57, kalman wrote:
> Dealing with script contexts isn't right either. Are there not other methods
on
> RenderViewOberver which might help?

As far as I can tell, willReleaseScriptContext is the only viable alternative. I
saw that the existence of an isolated world implied that a main world exists, so
it suffices to check whether the frame matches and world_id == 0 (main world).

willReleaseScriptContext was the only other suitable notification that I found;
see https://cs.chromium.org/"disposeContext(DetachGlobal);" (if you follow the
callers, you'll see that you eventually end up at the same code path as
replaceDocumentWhileExecutingJavaScriptURL; if you click through, then you'll
end up at the willReleaseScriotContext notification).

I recall (could be wrong, it was late) that replacing RenderViewObserver with
RenderFrameObserver was one of the objectives (from memory: to support OOP
frames?), so replacing RVO with RFO would align with that goal, right? (assuming
that my recollections of what I've read are correct)

[not at google - send to devlin, 2014-10-30 00:40:05]

kalman@chromium.org changed reviewers:
+ creis@chromium.org

[not at google - send to devlin, 2014-10-30 00:40:05]

I'll think through your comments tomorrow, but +creis on the OOP issue.

[not at google - send to devlin, 2014-10-30 14:57:43]

On 2014/10/30 00:34:58, robwu wrote:
> On 2014/10/30 00:20:57, kalman wrote:
> > Dealing with script contexts isn't right either. Are there not other methods
> on
> > RenderViewOberver which might help?
> 
> As far as I can tell, willReleaseScriptContext is the only viable alternative.
I
> saw that the existence of an isolated world implied that a main world exists,
so
> it suffices to check whether the frame matches and world_id == 0 (main world).

I'm not sure this is necessarily true? Try on example.com which runs no scripts.

> 
> willReleaseScriptContext was the only other suitable notification that I
found;
> see https://cs.chromium.org/%22disposeContext(DetachGlobal);%22 (if you follow
the
> callers, you'll see that you eventually end up at the same code path as
> replaceDocumentWhileExecutingJavaScriptURL; if you click through, then you'll
> end up at the willReleaseScriotContext notification).
> 
> I recall (could be wrong, it was late) that replacing RenderViewObserver with
> RenderFrameObserver was one of the objectives (from memory: to support OOP
> frames?), so replacing RVO with RFO would align with that goal, right?
(assuming
> that my recollections of what I've read are correct)

Regardless of whether the code path is right (it may be, I haven't checked), it
seems like a conflation of document lifetime with script context lifetime. But,
then arguably not, given this is potentially a javascript: specific bug. But
javascript: URLs have always been strange beasts. Ok.

[Charlie Reis, 2014-10-30 18:38:09]

creis@chromium.org changed reviewers:
+ dcheng@chromium.org, japhet@chromium.org

[Charlie Reis, 2014-10-30 18:38:10]

[+japhet,dcheng for Blink question]

On 2014/10/30 14:57:43, kalman wrote:
> On 2014/10/30 00:34:58, robwu wrote:
> > On 2014/10/30 00:20:57, kalman wrote:
> > > Dealing with script contexts isn't right either. Are there not other
methods
> > on
> > > RenderViewOberver which might help?
> > 
> > As far as I can tell, willReleaseScriptContext is the only viable
alternative.
> I
> > saw that the existence of an isolated world implied that a main world
exists,
> so
> > it suffices to check whether the frame matches and world_id == 0 (main
world).
> 
> I'm not sure this is necessarily true? Try on http://example.com which runs no
scripts.
> 
> > 
> > willReleaseScriptContext was the only other suitable notification that I
> found;
> > see https://cs.chromium.org/%22disposeContext(DetachGlobal);%22 (if you
follow
> the
> > callers, you'll see that you eventually end up at the same code path as
> > replaceDocumentWhileExecutingJavaScriptURL; if you click through, then
you'll
> > end up at the willReleaseScriotContext notification).
> > 
> > I recall (could be wrong, it was late) that replacing RenderViewObserver
with
> > RenderFrameObserver was one of the objectives (from memory: to support OOP
> > frames?), so replacing RVO with RFO would align with that goal, right?
> (assuming
> > that my recollections of what I've read are correct)
> 
> Regardless of whether the code path is right (it may be, I haven't checked),
it
> seems like a conflation of document lifetime with script context lifetime.
But,
> then arguably not, given this is potentially a javascript: specific bug. But
> javascript: URLs have always been strange beasts. Ok.

We do plan to remove RenderViewObserver with RenderFrameObserver, so moving over
is a good idea in general.

That said, I'm not sure that inferring whether the document has been cleared
from willReleaseScriptContext is right or not.  We can always add a more
explicit notification (from Blink) of the case you're looking for if we need
one.

Can you check in with Nate or Daniel about what you're trying to achieve?

[dcheng, 2014-10-30 21:48:56]

"These events do not trigger any document load notifications"

Nate, will your load refactoring stuff fix this down the road? Or is this
something that would trigger behavior that's not spec compliant?

[Nate Chapin, 2014-10-30 22:09:41]

On 2014/10/30 21:48:56, dcheng wrote:
> "These events do not trigger any document load notifications"
> 
> Nate, will your load refactoring stuff fix this down the road? Or is this
> something that would trigger behavior that's not spec compliant?

Which notifications? I can't find anything in the spec to implying that
documents created by javascript: urls shouldn't fire a load event, and I don't
immediately see anywhere in the code that would prevent it.

[robwu, 2014-10-30 22:15:06]

On 2014/10/30 14:57:43, kalman wrote:
> On 2014/10/30 00:34:58, robwu wrote:
> > On 2014/10/30 00:20:57, kalman wrote:
> > > Dealing with script contexts isn't right either. Are there not other
methods
> > on
> > > RenderViewOberver which might help?
> > 
> > As far as I can tell, willReleaseScriptContext is the only viable
alternative.
> I
> > saw that the existence of an isolated world implied that a main world
exists,
> so
> > it suffices to check whether the frame matches and world_id == 0 (main
world).
> 
> I'm not sure this is necessarily true? Try on http://example.com which runs no
scripts.

This is always true for javascript:-URLs, because there is always a main world
at the initial empty document (because of the JS code that is being evaluated).

> Regardless of whether the code path is right (it may be, I haven't checked),
it
> seems like a conflation of document lifetime with script context lifetime.
But,
> then arguably not, given this is potentially a javascript: specific bug. But
> javascript: URLs have always been strange beasts. Ok.

This is only to deal with javascript:-URLs, and afaik does not have negative
side effects on other use cases (unlike the present patch). Using this script
notification seems to be low-risk, but if there are better options available,
I'll try them out.

[not at google - send to devlin, 2014-10-30 22:19:46]

You just need to be careful because script lifetime != frame lifetime, and the
invalidation logic works with frames not scripts.

[robwu, 2014-11-01 00:15:09]

Is ScriptInjectionManager designed using the assumption that every frame is
within the same renderer process? I started to convert
ScriptInjectionManager::RVOHelper to be a RenderFrameObserver. After compiling
the result, chrome.tabs.executeScript did not work any more because Handler
(used by ScriptExecutor::ExecuteScript) sends the message to
|web_contents->GetRenderViewHost()|. When I looked for the RenderFrame
equivalent, I noticed that there were multiple methods that return a
|RenderFrameHost*|. That makes me think that the conversion from RenderView to
RenderFrame for content scripts is far from trivial. Are these findings correct?
What is the best way to get a WillReleaseScriptContext notification in
ScriptInjectionManager::RVOHelper?

[dcheng, 2014-11-01 00:19:57]

On 2014/11/01 at 00:15:09, rob wrote:
> Is ScriptInjectionManager designed using the assumption that every frame is
within the same renderer process? I started to convert
ScriptInjectionManager::RVOHelper to be a RenderFrameObserver. After compiling
the result, chrome.tabs.executeScript did not work any more because Handler
(used by ScriptExecutor::ExecuteScript) sends the message to
|web_contents->GetRenderViewHost()|. When I looked for the RenderFrame
equivalent, I noticed that there were multiple methods that return a
|RenderFrameHost*|. That makes me think that the conversion from RenderView to
RenderFrame for content scripts is far from trivial. Are these findings correct?
What is the best way to get a WillReleaseScriptContext notification in
ScriptInjectionManager::RVOHelper?

Before you do this conversion, I still don't understand what load notifications
are not getting dispatched for javascript: URLs. WillReleaseScriptContext()
doesn't really seem like the best place to do this sort of work.

[robwu, 2014-11-01 00:29:04]

On 2014/11/01 00:19:57, dcheng wrote:
> Before you do this conversion, I still don't understand what load
notifications
> are not getting dispatched for javascript: URLs. WillReleaseScriptContext()
> doesn't really seem like the best place to do this sort of work.

ScriptInjectionManager::RVOHelper cleans up the frame->document state/pending
scripts mapping when InvalidateFrame is called (use Ctrl+F at
https://chromium.googlesource.com/chromium/src/+/9168b2fe3e30afa4fd9a9500ba07...).
This always works, except for javascript:-URLs that return a value. This is
because this is not treated as a navigation.

See
https://chromium.googlesource.com/chromium/blink/+/5da5b5931fe82baf6affd57aae...
(you can easily follow the calls (e.g. using Codesearch) to see that this
special case does not behave as a navigation.)

Consequently, the ScriptInjectionManager is not notified when the transition
from document -> JavaScript result occurs, which prevents content scripts from
being inserted again (this is the bug linked with this CL).

[robwu, 2014-11-04 17:00:05]

rob@robwu.nl changed reviewers:
+ jam@chromium.org

[robwu, 2014-11-04 17:00:06]

+jam

In this CL, I added the WillReleaseScriptContext notification to
RenderViewObserver, because I need this notification in
extensions/renderer/script_injection_manager.cc to fix a bug. For context, see
the CL description and bug report.

Converting the existing code to use RenderFrame instead would be a lot of
effort, and would certainly delay the fix to the bug. Could you review my
few-line changes to content/ and tell me whether the change is acceptable for
the time being? If not, do you have any alternative suggestions?

[dcheng, 2014-11-04 22:40:45]

Do we still need to call InvalidateFrame in DidStartProvisionalLoad with this
change? It seems redundant to have both (since we should always call
setDOMWindow when committing a navigation). It also seems incorrect to have
called it in DidStartProvisionalLoad to begin with, but perhaps I'm missing
something.

https://codereview.chromium.org/684143002/diff/100001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/684143002/diff/100001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:118: if (world_id == 0) {
0 should be a named constant somewhere. It's not clear from inspection that the
main world ID is 0 (I only happen to know that right now because I looked
through
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
last night).

[robwu, 2014-11-04 22:46:42]

On 2014/11/04 22:40:45, dcheng wrote:
> Do we still need to call InvalidateFrame in DidStartProvisionalLoad with this
> change? It seems redundant to have both (since we should always call
> setDOMWindow when committing a navigation). It also seems incorrect to have
> called it in DidStartProvisionalLoad to begin with, but perhaps I'm missing
> something.

The script notification is not triggered when the page has no scripts, so the
new notification cannot replace another one. And I'm not entirely convinced that
DidStartProvisionalLoad is correct, I suspect that this notification is the
cause of https://crbug.com/400900.

> 
>
https://codereview.chromium.org/684143002/diff/100001/extensions/renderer/scr...
> File extensions/renderer/script_injection_manager.cc (right):
> 
>
https://codereview.chromium.org/684143002/diff/100001/extensions/renderer/scr...
> extensions/renderer/script_injection_manager.cc:118: if (world_id == 0) {
> 0 should be a named constant somewhere. It's not clear from inspection that
the
> main world ID is 0 (I only happen to know that right now because I looked
> through
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> last night).

I'll turn 0 into a constant after jam approves the use of
WillReleaseScriptContext.

[jam, 2014-11-06 03:13:25]

content lgtm

[dcheng, 2014-11-06 03:20:15]

On 2014/11/06 03:13:25, jam wrote:
> content lgtm

It still feels unfortunate: having this here means we double invalidate for most
normal loads, right?

[robwu, 2014-11-06 15:00:45]

On 2014/11/06 03:20:15, dcheng wrote:
> On 2014/11/06 03:13:25, jam wrote:
> > content lgtm
> 
> It still feels unfortunate: having this here means we double invalidate for
most
> normal loads, right?

Yep, that's indeed a bit unfortunate.
I have added a new notification in Blink specifically for the javascript:'...'
navigation, let's see if it passes review:
https://codereview.chromium.org/710443002/

[robwu, 2014-11-11 14:47:58]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-11-11 14:48:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684143002/120001

[commit-bot: I haz the power, 2014-11-11 15:45:05]

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

[commit-bot: I haz the power, 2014-11-11 15:45:06]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dcheng, 2014-11-11 16:50:16]

https://codereview.chromium.org/684143002/diff/120001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/684143002/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:2361: DCHECK(!frame_ || frame_ == frame);
This assert should be more aggressive. We should never call this and pass a null
frame parameter.

[robwu, 2014-11-11 21:43:59]

https://codereview.chromium.org/684143002/diff/120001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/684143002/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:2361: DCHECK(!frame_ || frame_ == frame);
On 2014/11/11 16:50:16, dcheng wrote:
> This assert should be more aggressive. We should never call this and pass a
null
> frame parameter.

This DCHECK does not test whether the passed-in pointer is a non-null pointer,
but it shows that frame ought to be equal to |frame_| if set.
This whole class assumes that the passed-in WebLocalFrame* pointer is non-null,
so I don't think that we need a CHECK(!frame);
If you look at src/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp, it
becomes obvious that that class also assumes that m_webFrame is always non-null,
so again I see no point im adding a stronger assertion here.

[dcheng, 2014-11-11 22:07:02]

On 2014/11/11 at 21:43:59, rob wrote:
>
https://codereview.chromium.org/684143002/diff/120001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/684143002/diff/120001/content/renderer/render...
> content/renderer/render_frame_impl.cc:2361: DCHECK(!frame_ || frame_ ==
frame);
> On 2014/11/11 16:50:16, dcheng wrote:
> > This assert should be more aggressive. We should never call this and pass a
null
> > frame parameter.
> 
> This DCHECK does not test whether the passed-in pointer is a non-null pointer,
but it shows that frame ought to be equal to |frame_| if set.
> This whole class assumes that the passed-in WebLocalFrame* pointer is
non-null, so I don't think that we need a CHECK(!frame);
> If you look at src/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp, it
becomes obvious that that class also assumes that m_webFrame is always non-null,
so again I see no point im adding a stronger assertion here.

I'm saying that the check should be:
DCHECK(frame_ == frame);
Why would we ever have !frame_?

[robwu, 2014-11-11 22:26:30]

On 2014/11/11 22:07:02, dcheng wrote:
> I'm saying that the check should be:
> DCHECK(frame_ == frame);
> Why would we ever have !frame_?

The DCHECK(!frame || frame_ == frame) pattern is all over this file, so I guess
that there must be a reason for it (i.e. RenderFrameImpl::SetWebFrame not called
before the method is called) (I don't know the reason, I haven't checked).

[dcheng, 2014-11-11 22:28:45]

On 2014/11/11 at 22:26:30, rob wrote:
> On 2014/11/11 22:07:02, dcheng wrote:
> > I'm saying that the check should be:
> > DCHECK(frame_ == frame);
> > Why would we ever have !frame_?
> 
> The DCHECK(!frame || frame_ == frame) pattern is all over this file, so I
guess that there must be a reason for it (i.e. RenderFrameImpl::SetWebFrame not
called before the method is called) (I don't know the reason, I haven't
checked).

Legacy reasons. There are a couple places that only do DCHECK_EQ(frame_, frame),
and if we can do that here, that would be ideal.

[robwu, 2014-11-15 00:48:42]

kalman: Do you approve of patch set 9?

This change is needed because calling chrome.tabs.update({url: 'javascript:void
...'}) right after chrome.tabs.create results in the following event sequence:

1. HandleExecuteCode called and appends ScriptInjection to pending_injections_
2. DidCreateNewDocument is triggered.
3. (previously: InvalidateFrame is called, which clears pending_injections).
   (after PS 9: frame is not in frame_statuses_, so InvalidateFrame(frame) is
not called)

[robwu, 2014-11-15 10:04:17]

On 2014/11/11 22:28:45, dcheng wrote:
> On 2014/11/11 at 22:26:30, rob wrote:
> > On 2014/11/11 22:07:02, dcheng wrote:
> > > I'm saying that the check should be:
> > > DCHECK(frame_ == frame);
> > > Why would we ever have !frame_?
> > 
> > The DCHECK(!frame || frame_ == frame) pattern is all over this file, so I
> guess that there must be a reason for it (i.e. RenderFrameImpl::SetWebFrame
not
> called before the method is called) (I don't know the reason, I haven't
> checked).
> 
> Legacy reasons. There are a couple places that only do DCHECK_EQ(frame_,
frame),
> and if we can do that here, that would be ideal.

I've reverted this suggested DCHECK change, because it caused some tests to
fail:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
CrossProcessFrameTreeBrowserTest.CreateCrossProcessSubframeProxies
SitePerProcessBrowserTest.NavigateRemoteFrame
SitePerProcessBrowserTest.ProxyCreationSkipsSubtree
SitePerProcessBrowserTest.CrossSiteIframe
SitePerProcessAccessibilityBrowserTest.CrossSiteIframeAccessibility

This is caused by the fact that in render_frame_impl.cc,
  render_frame->SetWebFrame(web_frame);
is called after
  web_frame = parent_web_frame->createLocalChild("", render_frame);
but createLocalChild already triggers didCreateNewDocument:

 #8 0x0000008d9f12 content::RenderFrameImpl::didCreateNewDocument()
 #9 0x0000008da01f content::RenderFrameImpl::didCreateNewDocument()
 #10 0x0000032ce05b blink::FrameLoaderClientImpl::didCreateNewDocument()
 #11 0x000003b1ef15 blink::FrameLoader::didBeginDocument()
 #12 0x000003b1260c blink::DocumentLoader::createWriterFor()
 #13 0x000003b122f2 blink::DocumentLoader::ensureWriter()
 #14 0x000003b10935 blink::DocumentLoader::commitData()
 #15 0x000003b10504 blink::DocumentLoader::finishedLoading()
 #16 0x000003b13374 blink::DocumentLoader::maybeLoadEmpty()
 #17 0x000003b134e2 blink::DocumentLoader::startLoadingMainResource()
 #18 0x000003b1d002 blink::FrameLoader::init()
 #19 0x000003266f6c blink::LocalFrame::init()
 #20 0x000003263770 blink::WebLocalFrameImpl::initializeCoreFrame()
 #21 0x00000328e6e5 blink::WebRemoteFrameImpl::createLocalChild()
 #22 0x0000008c7ce7 content::RenderFrameImpl::CreateFrame()
 #23 0x00000090c783 content::RenderThreadImpl::OnCreateNewFrame()
 #24 0x0000008b2f6a DispatchToMethod<>()
 #25 0x000000910c87 FrameMsg_NewFrame::Dispatch<>()

[not at google - send to devlin, 2014-11-17 22:20:05]

lgtm

https://codereview.chromium.org/684143002/diff/200001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/684143002/diff/200001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:110: InvalidateFrame(frame);
Let's just call InvalidateFrame(frame) and forget about the check, everything
should work fine. None of the other methods that call this worry about whether
or not the frame was actually used.

[not at google - send to devlin, 2014-11-17 22:20:17]

(when dcheng is happy)

[dcheng, 2014-11-17 22:28:20]

On 2014/11/17 at 22:20:17, kalman wrote:
> (when dcheng is happy)

Given that the DCHECK change I suggested is currently impractical, I have no
further comments.

[robwu, 2014-11-17 22:34:43]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-11-17 22:37:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/684143002/200001

[commit-bot: I haz the power, 2014-11-17 23:56:32]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2014-11-17 23:57:32]

Patchset 10 (id:??) landed as
https://crrev.com/5ef11ff1b68256eb46ce90f5f14a7bd8cacb51a8
Cr-Commit-Position: refs/heads/master@{#304522}