Bug 56608 – Regression(r80861): fast/dom/Window/window-lookup-precedence.html started failing on Windows Chromium.

• Component: DOM
• Status: RESOLVED FIXED

[David Levin]

You can see the failures here: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fdom%2FWindow%2Fwindow-lookup-precedence.html&group=%40ToT%20-%20chromium.org

It is a little confusing at first because the items mentioned in the output don't appear in the test at all. Upon closer inspection, the iframe in the test points to a local directory. It appears that chromium forms this view by injecting javascript code into the page. This code is the thing that is having problems.

[David Levin]

You can repro the issue on OSX as well but there is already a failure in test expectations for this test so I guess it has other problems on that platform as well.

I find it odd that Linux seems unaffected.

One simple way to get a call stack for the issue is to set a breakpoint on the first constructor for ConsoleMessage in Source/WebCore/inspector/ConsoleMessage.cpp

In case it is helpful, here's the call stack that I got:

#0	0x012657af in WebCore::ConsoleMessage::ConsoleMessage(WebCore::MessageSource, WebCore::MessageType, WebCore::MessageLevel, WTF::String const&, unsigned int, WTF::String const&) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../inspector/ConsoleMessage.cpp:57
#1	0x012848e2 in WebCore::InspectorConsoleAgent::addMessageToConsole(WebCore::MessageSource, WebCore::MessageType, WebCore::MessageLevel, WTF::String const&, unsigned int, WTF::String const&) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../inspector/InspectorConsoleAgent.cpp:128
#2	0x012ae805 in WebCore::InspectorInstrumentation::addMessageToConsoleImpl(WebCore::InspectorAgent*, WebCore::MessageSource, WebCore::MessageType, WebCore::MessageLevel, WTF::String const&, unsigned int, WTF::String const&) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../inspector/InspectorInstrumentation.cpp:582
#3	0x01343db3 in WebCore::InspectorInstrumentation::addMessageToConsole(WebCore::Page*, WebCore::MessageSource, WebCore::MessageType, WebCore::MessageLevel, WTF::String const&, unsigned int, WTF::String const&) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../inspector/InspectorConsoleInstrumentation.h:53
#4	0x01343164 in WebCore::Console::addMessage(WebCore::MessageSource, WebCore::MessageType, WebCore::MessageLevel, WTF::String const&, unsigned int, WTF::String const&, WTF::PassRefPtr<WebCore::ScriptCallStack>) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../page/Console.cpp:151
#5	0x010fc3ea in WebCore::Document::addMessage(WebCore::MessageSource, WebCore::MessageType, WebCore::MessageLevel, WTF::String const&, unsigned int, WTF::String const&, WTF::PassRefPtr<WebCore::ScriptCallStack>) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../dom/Document.cpp:4706
#6	0x010fc12a in WebCore::Document::logExceptionToConsole(WTF::String const&, int, WTF::String const&, WTF::PassRefPtr<WebCore::ScriptCallStack>) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../dom/Document.cpp:2301
#7	0x01185e3c in WebCore::ScriptExecutionContext::reportException(WTF::String const&, int, WTF::String const&, WTF::PassRefPtr<WebCore::ScriptCallStack>) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../dom/ScriptExecutionContext.cpp:332
#8	0x00f9b617 in WebCore::v8UncaughtExceptionHandler(v8::Handle<v8::Message>, v8::Handle<v8::Value>) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../bindings/v8/V8DOMWindowShell.cpp:121
#9	0x00b6e0e4 in v8::internal::MessageHandler::ReportMessage(v8::internal::MessageLocation*, v8::internal::Handle<v8::internal::Object>) at /Users/levin/src/chrome/src/v8/tools/gyp/../../src/messages.cc:127
#10	0x00c64e07 in v8::internal::Top::ReportPendingMessages() at /Users/levin/src/chrome/src/v8/tools/gyp/../../src/top.cc:1015
#11	0x00aab84e in v8::internal::Invoke(bool, v8::internal::Handle<v8::internal::JSFunction>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Object***, bool*) at /Users/levin/src/chrome/src/v8/tools/gyp/../../src/execution.cc:108
#12	0x00aabd2d in v8::internal::Execution::Call(v8::internal::Handle<v8::internal::JSFunction>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Object***, bool*) at /Users/levin/src/chrome/src/v8/tools/gyp/../../src/execution.cc:128
#13	0x00a51bc2 in v8::Script::Run() at /Users/levin/src/chrome/src/v8/tools/gyp/../../src/api.cc:1314
#14	0x00faf19f in WebCore::V8Proxy::runScript(v8::Handle<v8::Script>, bool) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../bindings/v8/V8Proxy.cpp:417
#15	0x00faf550 in WebCore::V8Proxy::evaluate(WebCore::ScriptSourceCode const&, WebCore::Node*) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../bindings/v8/V8Proxy.cpp:371
#16	0x00f823d6 in WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../bindings/v8/ScriptController.cpp:235
#17	0x01183dee in WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../dom/ScriptElement.cpp:256
#18	0x01184ca7 in WebCore::ScriptElement::prepareScript(WTF::TextPosition<WTF::OneBasedNumber> const&, WebCore::ScriptElement::LegacyTypeSupport) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../dom/ScriptElement.cpp:213
#19	0x0027f409 in WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition<WTF::OneBasedNumber> const&) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../html/parser/HTMLScriptRunner.cpp:289
#20	0x00280062 in WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition<WTF::OneBasedNumber> const&) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../html/parser/HTMLScriptRunner.cpp:173
#21	0x00272f14 in WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../html/parser/HTMLDocumentParser.cpp:205
#22	0x00272f81 in WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../html/parser/HTMLDocumentParser.cpp:216
#23	0x0027366b in WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../html/parser/HTMLDocumentParser.cpp:255
#24	0x00273964 in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../html/parser/HTMLDocumentParser.cpp:175
#25	0x00273ead in WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../html/parser/HTMLDocumentParser.cpp:343
#26	0x010f2812 in WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, int, bool) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../dom/DecodedDataDocumentParser.cpp:54
#27	0x012e0369 in WebCore::DocumentWriter::addData(char const*, int, bool) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../loader/DocumentWriter.cpp:201
#28	0x012d54f3 in WebCore::DocumentLoader::commitData(char const*, int) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../loader/DocumentLoader.cpp:317
#29	0x00099660 in WebKit::WebFrameImpl::commitDocumentData(char const*, unsigned long) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:1062
#30	0x00064f90 in WebKit::FrameLoaderClientImpl::committedLoad(WebCore::DocumentLoader*, char const*, int) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1063
#31	0x012d55cd in WebCore::DocumentLoader::commitLoad(char const*, int) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../loader/DocumentLoader.cpp:303
#32	0x012d5642 in WebCore::DocumentLoader::receivedData(char const*, int) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../loader/DocumentLoader.cpp:329
#33	0x01301b36 in WebCore::MainResourceLoader::addData(char const*, int, bool) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../loader/MainResourceLoader.cpp:161
#34	0x01312275 in WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../loader/ResourceLoader.cpp:279
#35	0x0130115d in WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../loader/MainResourceLoader.cpp:446
#36	0x01311a18 in WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../loader/ResourceLoader.cpp:430
#37	0x000798ae in WebCore::ResourceHandleInternal::didReceiveData(WebKit::WebURLLoader*, char const*, int) at /Users/levin/src/chrome/src/third_party/WebKit/Source/WebKit/chromium/src/ResourceHandle.cpp:173
#38	0x01b21077 in webkit_glue::WebURLLoaderImpl::Context::OnReceivedData(char const*, int) at /Users/levin/src/chrome/src/webkit/support/../glue/weburlloader_impl.cc:609

[Tony Gentilcore]

Created attachment 86233 [details]
Patch

[David Levin]

I'm confused about why this is the correct fix.

When I look at the output, I see that code within the frame seems to fail to run correctly. Why is this affected by whether the frame has loaded before setting a new location?

[Mihai Parparita]

Comment on attachment 86233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86233&action=review

> LayoutTests/fast/dom/Window/window-lookup-precedence.html:65
> +<iframe src="about:blank" onload="test()"></iframe>

Does this need jsTestIsAsync = true and an explicit finishTest() call now?

[Adam Barth]

(In reply to comment #4)
> (From update of attachment 86233 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86233&action=review
> 
> > LayoutTests/fast/dom/Window/window-lookup-precedence.html:65
> > +<iframe src="about:blank" onload="test()"></iframe>
> 
> Does this need jsTestIsAsync = true and an explicit finishTest() call now?

about:blank calls onload synchronously, because it's awesome.

[Mihai Parparita]

(In reply to comment #5)
> about:blank calls onload synchronously, because it's awesome.

But if it's synchronous, then why did Tony need to make this change in the first place (he said he's going to look into that, but pointers might be appreciated).

[David Levin]

Comment on attachment 86233 [details]
Patch

r- to move this out of the review queue. There are a lot of open questions mentioned in this bug which should be answered before this can be reviewed. Once they are answered if no changes are needed, feel free to mark this r? again.

[Tony Gentilcore]

My original patch wouldn't have fixed this. I assumed it was the same problem as some other tests without actually reproducing it.

Now I have the problem reproducing. It looks like the test (unintentionally) is setting window.location which is navigating the subframe. I believe the fix is going to involve skipping "location" in the same way the test skips "showModalDialog", but I'm still verifying.

[David Levin]

(In reply to comment #8)
> My original patch wouldn't have fixed this. I assumed it was the same problem as some other tests without actually reproducing it.
> 
> Now I have the problem reproducing. It looks like the test (unintentionally) is setting window.location which is navigating the subframe. I believe the fix is going to involve skipping "location" in the same way the test skips "showModalDialog", but I'm still verifying.

Isn't that hiding the real issue that this malfunctions when it didn't before? Why is it malfunctioning?

[Tony Gentilcore]

Created attachment 88885 [details]
Patch

[David Levin]

Comment on attachment 88885 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88885&action=review

> LayoutTests/fast/dom/Window/window-lookup-precedence.html:41
> +    // "location" change would result in unwanted subframe navigation.

Why is that bad? And why does it cause problems (when you look at the output it looks like something deeper is busted)?

[Tony Gentilcore]

(In reply to comment #11)
> (From update of attachment 88885 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88885&action=review
> 
> > LayoutTests/fast/dom/Window/window-lookup-precedence.html:41
> > +    // "location" change would result in unwanted subframe navigation.
> 
> Why is that bad? And why does it cause problems (when you look at the output it looks like something deeper is busted)?

Okay, finally got to the bottom of this.

The line of the test that does "subframe[name] = name" effectively does "window.frames[0].location = 'location'" when name == "location". On Safari that doesn't navigate the frame, but rather opens LayoutTests/fast/dom/Window/location/ in the finder. Chromium, OTOH, navigates the frame to file:///work/WebKit/LayoutTests/fast/dom/Window/location/ which loads chromium's directory browser.

Now, chromium's directory browser is generated by net/base/net_util.cc:GetDirectoryListingHeader() and GetDirectoryListingEntry(). It generates the page in two parts, the first is a static resource that has JS which defines methods like addRow() and start(), the second are dynamically written calls to those methods. In DRT, the header resource isn't available, so those methods really aren't defined. This is the relevant code from net_util.cc:

  static const base::StringPiece header(
      NetModule::GetResource(IDR_DIR_HEADER_HTML));
  // This can be null in unit tests.
  DLOG_IF(WARNING, header.empty()) <<
      "Missing resource: directory listing header";

Prior to my patch, there was no parser yield, so the test would finish before the console output could show up. After my patch, there was a yield which allowed the console output to appear in the test output. The key thing to notice is that in both cases, there is the same console output. This can be verified locally by rolling out my patch and modifying the test to add this at the end of the last script block:

layoutTestController.waitUntilDone();
setTimeout(function() {
  layoutTestController.notifyDone();
}, 1000);

So, I think my patch which prevents chromium from navigating to the directory browsing page is the correct fix. The subframe doesn't navigate on Safari, and I suspect the original author of the test didn't intend for the test to navigate the subframe while checking that list of properties. Another option would be to add a waitUntilDone/notifyDone and expect the console errors to be there for chromium.

[David Levin]

Comment on attachment 88885 [details]
Patch

Awesome! Thanks for your thoroughness.

One last question are we loosing coverage in this test by skipping location?
And if so, is there any way to fix the test without reducing coverage like this?

[David Levin]

Changing the bug title since the patch appears to be broader than chromium (changing the layout test).

[Tony Gentilcore]

Created attachment 89143 [details]
patch

This avoids the navigation by only skipping setting window.location on the subframe, but it still checks the property.

[Tony Gentilcore]

Created attachment 89263 [details]
patch

Oops, I lost the ChangeLog in my previous patch. I'm having to upload these manually because webkit-patch doesn't notice the moved file.

[Tony Gentilcore]

Hi Pam, I know you added this test a long time ago, but would you mind taking a look at this patch to see if it keeps the intent of the test in tact?

The key question is whether it is expected that setting subframe.location should navigate the subframe.

[Pam Greene (IRC:pamg)]

This is one of the tests moved from Chromium into Webkit long ago. I handled the move, but it was originally written by Mads. Mads, can you comment on this change?

[Mads Ager]

I am pretty sure I did not have subframe navigation in mind when writing this test. I was trying to verify that the precedence of various things on the global object behaved correctly. Seems like a good thing to remove the subframe navigation. Thanks!

[WebKit Commit Bot]

Comment on attachment 89263 [details]
patch

Clearing flags on attachment: 89263

Committed r83726: <http://trac.webkit.org/changeset/83726>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.