Bug 45430 – http/tests/navigation/anchor-frames.html doesn't pass when not run in isolation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mihai Parparita]

http/tests/navigation/anchor-frames.html doesn't pass with TestShell

[Mihai Parparita]

Created attachment 66978 [details]
Patch

[Dirk Pranke]

this change LGTM. James, do you feel like rubber-stamping this?

[James Robinson]

Comment on attachment 66978 [details]
Patch

I'd strongly prefer to know when TestShell is updating the scroll offset and having the test code run off of that.  Running it in a setTimeout() might be racy depending on exactly when test shell updates the scroll offsets and is unreliable for a test.

[James Robinson]

Alternately, decide we don't care about this test for TestShell and add it to the expectations.  That makes me a little nervous but might a good option if it seems it will take a long time to debug what TestShell is doing.

[James Robinson]

This test is currently failing on mac's DumpRenderTree as well (leopard and snow leopard).  Sure this is a test shell problem?

[Mihai Parparita]

Re-titling, since this appears to fail with the regular DRT (though I can only reproduce locally when using --iterations 2). The setTimeout wrapper does seem to fix that, but I'd like to understand why better first.

[Mihai Parparita]

Not sure how the test ever worked. In FrameLoader::finishedParsing() we do things in this order:
1. Call FrameLoader::checkCompleted (which triggers the onload handler via FrameLoader::checkCallImplicitClose -> Document::implicitClose -> Document::dispatchWindowLoadEvent)
2. Call m_frame->view()->scrollToFragment(m_URL)

So onload always fires before we scroll to the fragment.

I think the original patch on this bug is the way to go, since the setTimeout(0) will definitely happen after both 1 and 2 above, so we'll always be scrolled.

I'm going to be offline for a couple of hours. James/Dirk, feel free to either land this patch or back out r67033.

[James Robinson]

Ahh, I see.  So this patch is fine, then, since the setTimeout() will _always_ happen after the scroll.

[James Robinson]

Comment on attachment 66978 [details]
Patch

I'll land this to fix the bots.

[James Robinson]

Comment on attachment 66978 [details]
Patch

Clearing flags on attachment: 66978

Committed r67051: <http://trac.webkit.org/changeset/67051>

[James Robinson]

All reviewed patches have been landed.  Closing bug.

[Mihai Parparita]

(In reply to comment #7)
> Not sure how the test ever worked. 

I think I understand this better. The test has external resources (two scripts and a stylesheet). IIRC, stylesheets do not block, so in the uncached case, checkCompleted returns early (because of the numRequests(m_frame->document()) check) and we end up calling scrollToFragment before onload is dispatched. However, in the cached case, the resources are (presumably) loaded synchronously from the cache, so we have no outstanding requests.

This is why the test passed when run in isolation (or even when running all of the http/tests/navigation tests, since it's the first one to use those resources in there), but failed when run twice or when being run with all the other tests on the bots.