New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 660872 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 477150



Sign in to add a comment

updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks doesn't work with remote frames

Project Member Reported by lukasza@chromium.org, Oct 31 2016

Issue description

Repro steps via layout tests at ToT (d2c8b422):

    $ ninja -C out/gn blink_tests
    $ third_party/WebKit/Tools/Scripts/run-webkit-tests \
      --additional-drt-flag=--site-per-process --no-retry-failures -v -t gn \
      http/tests/security/mixedContent/insecure-plugin-in-iframe.html 

Note that without --site-per-process cmdline switch the test passes.


Expected test output:

CONSOLE WARNING: line 10: Mixed Content: The page at 'https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-plugin.html' was loaded over HTTPS, but requested an insecure plugin resource 'http://example.test:8000/security/mixedContent/resources/dummy.swf'. This content should also be served over HTTPS.
CONSOLE MESSAGE: Blink Test Plugin: initializing
This test loads a secure iframe that loads an insecure plugin. We should get a mixed content callback because the insecure plugin can script the secure origin.


Actual test output:

CONSOLE WARNING: Mixed Content: The page at 'https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-plugin.html' was loaded over HTTPS, but requested an insecure plugin resource 'http://example.test:8000/security/mixedContent/resources/dummy.swf'. This content should also be served over HTTPS.
CONSOLE MESSAGE: Blink Test Plugin: initializing
CONSOLE ERROR: line 10: Uncaught TypeError: Failed to execute 'updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks' on 'Internals': The node provided is neither a document nor an IFrame.
This test loads a secure iframe that loads an insecure plugin. We should get a mixed content callback because the insecure plugin can script the secure origin.


Diff:

No "line 10: " in the first line of test output.

Extra line of test output: CONSOLE ERROR: line 10: Uncaught TypeError: Failed to execute 'updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks' on 'Internals': The node provided is neither a document nor an IFrame.
 
Status: Available (was: Untriaged)
Cc: r...@opera.com
Summary: updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks doesn't work with remote frames (was: http/tests/security/mixedContent/insecure-plugin-in-iframe.html layout test fails in --site-per-process mode)
The first difference (lack of "line 10: " with OOPIFs) is kind of WAI.

I believe the second difference indicates that updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks doesn't work with remote frames (aka OOPIFs - out of process iframes):

third_party/WebKit/Source/core/testing/Internals.cpp:

void Internals::updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks(
    Node* node,
    ExceptionState& exceptionState) {
  Document* document = nullptr;
  if (!node) {
    document = contextDocument();
  } else if (node->isDocumentNode()) {
    document = toDocument(node);
  } else if (isHTMLIFrameElement(*node)) {
    // WE HAVE A PROBLEM HERE?  AFAIK contentDocument will be null for an OOPIF.
    document = toHTMLIFrameElement(*node).contentDocument();
  }

  if (!document) {
    exceptionState.throwTypeError(
        "The node provided is neither a document nor an IFrame.");
    return;
  }
  document->updateStyleAndLayoutIgnorePendingStylesheets(
      Document::RunPostLayoutTasksSynchronously);
}

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c709a831773d24b82e64888eae63e3dda9c9d66

commit 0c709a831773d24b82e64888eae63e3dda9c9d66
Author: lukasza <lukasza@chromium.org>
Date: Mon Oct 31 16:59:12 2016

Updating --site-per-process expectations for insecure-plugin-in-iframe.html

The test stopped crashing in --site-per-process mode after r428312.
A new bug has been opened for the remaining text diff in test output.

BUG= 582494 ,  660872 
NOTRY=true

Review-Url: https://codereview.chromium.org/2466593002
Cr-Commit-Position: refs/heads/master@{#428745}

[modify] https://crrev.com/0c709a831773d24b82e64888eae63e3dda9c9d66/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Comment 4 by r...@opera.com, Oct 31 2016

I don't understand why we'd need to call updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks() explicitly in the test. It's done on begin-frame regardlessly? Isn't that enough?

Cc: tkent@chromium.org
tkent@ - would you remember why you've added the call to updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks in third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-plugin-in-iframe.html in https://codereview.chromium.org/61763016?  As rune@ points out it does seem unneeded (i.e. the mixed content message appears even without this call).  Would it be okay to just remove the call from this particular test?

Comment 6 by tkent@chromium.org, Oct 31 2016

Comment 5: It was necessary to load a plugin in the IFRAME immediately.  I'm not sure if it is unnecessary now.  At worst, removing it might make the test flaky.


Comment 7 by r...@opera.com, Nov 1 2016

lukasza@: If you wrap the internals call in a try/catch, you will run the updateLayoutIgnore... in the LocalFrame case, and get the same expected output for the OOPIF case (it won't have an effect regardlessly).

Owner: lukasza@chromium.org
Status: Started (was: Available)
For http/tests/security/mixedContent/insecure-plugin-in-iframe.html, I think I'll just move the call to updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks from the parent into the child frame (so that the call always happens on a local document).  This is a cowardly^H^H^Hlow-risk CL that will hopefully pave way toward enabling this test with --site-per-process (although I agree that in principle we might want to investigate getting rid of that call altogether).

Once the above is done, I can try to dust off the CL (https://codereview.chromium.org/2191143002) that allows layout tests to strip/ignore console line numbers (tracked broadly by  issue 619662 ).
Fix proposed at https://codereview.chromium.org/2468953002
(it turns out that console line numbers can be tackled without going back to https://codereview.chromium.org/2191143002 for help).
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd8d931dc59e273273701feae744adff8714c27f

commit cd8d931dc59e273273701feae744adff8714c27f
Author: lukasza <lukasza@chromium.org>
Date: Tue Nov 01 23:15:21 2016

Make mixedContent/insecure-plugin-in-iframe.html test OOPIF-compatible.

The CL makes the following changes:

- Moves the call to
  internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks
  from the parent frame into the child frame.  This ensures that this
  method works on a local document - this is important because the
  method doesn't support OOPIFs.

- Delays the call to
  internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks
  via window.setTimeout.  This ensures that the mixed content message
  does not include the line number of the call (and so appears the same
  with an without OOPIFs).  The line number used to indicate which line
  called internals.updateLayoutIgnore...heetsAndRunPostLayoutTasks and
  this piece of information is artificial (and not essential to test
  for) - no real page will call this test-only window.internals method.

- To make sure the test finishes *after* the delayed call to
  internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks
  a pair of testRunner.waitUntilDone / testRunner.notifyDone calls
  has been added.  Note that the subframe used to post a "done"
  message back to the opener, but this message was never listened to
  (and so the postMessage call can be deleted).

BUG= 660872 

Review-Url: https://codereview.chromium.org/2468953002
Cr-Commit-Position: refs/heads/master@{#429147}

[modify] https://crrev.com/cd8d931dc59e273273701feae744adff8714c27f/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/cd8d931dc59e273273701feae744adff8714c27f/third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-plugin-in-iframe-expected.txt
[modify] https://crrev.com/cd8d931dc59e273273701feae744adff8714c27f/third_party/WebKit/LayoutTests/http/tests/security/mixedContent/insecure-plugin-in-iframe.html
[modify] https://crrev.com/cd8d931dc59e273273701feae744adff8714c27f/third_party/WebKit/LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-plugin.html

Status: Fixed (was: Started)
Hmmm... it seems that http/tests/security/mixedContent/insecure-plugin-in-iframe.html runs as expected now.  I am guessing that the CL from #c10 above fixed the problem and I just forgot to mark the bug as fixed.

Sign in to add a comment