Bug 20342 – REGRESSION: fast/dom/cssTarget-crash.html fails

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

fast/dom/cssTarget-crash.html fails on build.webkit.org and locally.

I think that is also what's causing fast/dom/defaultView.html to appear to be failing.

The failure may be related to <http://trac.webkit.org/changeset/35611>.

[David Kilzer (:ddkilzer)]

Test was added in in r29311.

http://trac.webkit.org/changeset/29311

Reading the source of the HTML test, I don't understand how it would ever stop (e.g., layoutTestController.notifyDone() would get called) after the form is submitted.

[Alexey Proskuryakov]

FWIW, this test works in Firefox, too (after replacing layoutTestController call with an alert(), of course).

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> FWIW, this test works in Firefox, too (after replacing layoutTestController
> call with an alert(), of course).

Hmm...need to understand why this should work, then.

[David Kilzer (:ddkilzer)]

(In reply to comment #0)
> I think that is also what's causing fast/dom/defaultView.html to appear to be
> failing.
> 
> The failure may be related to <http://trac.webkit.org/changeset/35611>.

That was the fix for Bug 20038.

That change was fall-out from <http://trac.webkit.org/changeset/35151> (Bug 13067).

I haven't had time to look at this yet.

[Sam Weinig]

<rdar://problem/6145826>

[David Kilzer (:ddkilzer)]

I'm close to an update for the layout test itself.

[David Kilzer (:ddkilzer)]

Created attachment 22777 [details]
Patch v1

Proposed fix.

[Alexey Proskuryakov]

Comment on attachment 22777 [details]
Patch v1

What about the fact that the test work on Firefox as is? This patch may fix the test results, but we still have a P1/regression bug, don't we?

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> (From update of attachment 22777 [details] [edit])
> What about the fact that the test work on Firefox as is? This patch may fix the
> test results, but we still have a P1/regression bug, don't we?

Please attach the modified test (with alerts) that "works" on Firefox 2/3.

I disagree that the original test should ever have worked--shouldn't a form submission cancel all setTimeout methods and stop JavaScript execution (assuming there's no "onsubmit" action)?

[Alexey Proskuryakov]

Created attachment 22779 [details]
modified test

(In reply to comment #9)
> Please attach the modified test (with alerts) that "works" on Firefox 2/3.

Hmm... Ok.

> I disagree that the original test should ever have worked--shouldn't a form
> submission cancel all setTimeout methods and stop JavaScript execution
> (assuming there's no "onsubmit" action)?

You can try with IE (I haven't), but for compatibility with Firefox, it clearly shoudn't.

[Alexey Proskuryakov]

In IE, this works when loading the test from network. When loading it from a local file, an alert never appears.

Shipping Safari behavior matched Firefox, while ToT matches IE on this test.

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> In IE, this works when loading the test from network. When loading it from a
> local file, an alert never appears.
> 
> Shipping Safari behavior matched Firefox, while ToT matches IE on this test.

So is Firefox saying, "submitting a form with no form elements is equivalent to loading the URL as if it were clicked on"?  That would certainly explain the FF behavior, although it just seems wrong on so many levels.  :)

[Alexey Proskuryakov]

Not necessarily - it could be a timing issue (given that the test behaves differently in IE and ToT WebKit depending on where it is loaded from).

[Alexey Proskuryakov]

So, I think that is almost OK to land, but it would be good to find out what exactly other browsers do.

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> So, I think that is almost OK to land, but it would be good to find out what
> exactly other browsers do.

Yes, i want to do some more research first.

[Eric Seidel (no email)]

Comment on attachment 22777 [details]
Patch v1

Marking as r+ based on ap's comments.  I trust David to finish his research and make an educated decision as to if this should really be landed or not.

[Sam Weinig]

Remember to take the test off the Skipped list when landing.

[Darin Adler]

Comment on attachment 22777 [details]
Patch v1

Clearing the review flag on this patch for now so it doesn't show up in the "reviewed, to be committed" list.

[Antonio Gomes]

(In reply to comment #15)
> (In reply to comment #14)
> > So, I think that is almost OK to land, but it would be good to find out what
> > exactly other browsers do.
> 
> Yes, i want to do some more research first.

@david: (sorry for this kind of annoying question but) do you plan to post here your research results for the desired behaviour ? and maybe move forward in this bug ...

[David Kilzer (:ddkilzer)]

(In reply to comment #19)
> @david: (sorry for this kind of annoying question but) do you plan to post here
> your research results for the desired behaviour ? and maybe move forward in
> this bug ...

Sorry, I haven't had time and this hasn't been a priority.  I'll try to get to it in the next week or so, but anyone else with a Windows PC and a Mac are welcome to do the research and post their results here.  :)

[Antonio Gomes]

> Sorry, I haven't had time and this hasn't been a priority.  I'll try to get to
> it in the next week or so, but anyone else with a Windows PC and a Mac are
> welcome to do the research and post their results here.  :)

david, anything I could help w/ with a linux box and qtwebkit build ?

we are facing a crash running this specific layouttest in qtwebkit (see below). I am not sure if on Mac it was crashing or just "failing"  though ...

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xb469a700 (LWP 23201)]
0xb6d610c8 in WebCore::Node::childNeedsStyleRecalc (this=0x8) at ../../../../WebCore/dom/Node.h:270
270	    bool childNeedsStyleRecalc() const { return m_childNeedsStyleRecalc; }
(gdb) 
(gdb) 
(gdb) bt
#0  0xb6d610c8 in WebCore::Node::childNeedsStyleRecalc (this=0x8) at ../../../../WebCore/dom/Node.h:270
#1  0xb6ddc903 in WebCore::Node::setNeedsStyleRecalc (this=0x98f1ca8, changeType=WebCore::FullStyleChange) at ../../../../WebCore/dom/Node.cpp:701
#2  0xb6d9291c in WebCore::Document::setCSSTarget (this=0x98db1d8, n=0x0) at ../../../../WebCore/dom/Document.cpp:2638
#3  0xb6fd7044 in WebCore::FrameLoader::gotoAnchor (this=0x9795b8c, name=@0xbffde9a8) at ../../../../WebCore/loader/FrameLoader.cpp:1583
#4  0xb6fd810b in WebCore::FrameLoader::gotoAnchor (this=0x9795b8c) at ../../../../WebCore/loader/FrameLoader.cpp:1198
#5  0xb707485c in WebCore::FrameView::layout (this=0x97a9450, allowSubtree=true) at ../../../../WebCore/page/FrameView.cpp:672
#6  0xb7074e03 in WebCore::FrameView::layoutTimerFired (this=0x97a9450) at ../../../../WebCore/page/FrameView.cpp:924
#7  0xb706c55f in WebCore::Timer<WebCore::FrameView>::fired (this=0x97a9518) at ../../../../WebCore/platform/Timer.h:98
#8  0xb710799b in WebCore::ThreadTimers::fireTimers (this=0x978f0f0, fireTime=1246599950.3275959, firingTimers=@0xbffdeb34)
    at ../../../../WebCore/platform/ThreadTimers.cpp:111
#9  0xb7108550 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x978f0f0) at ../../../../WebCore/platform/ThreadTimers.cpp:141
#10 0xb71085cf in WebCore::ThreadTimers::sharedTimerFired () at ../../../../WebCore/platform/ThreadTimers.cpp:122
#11 0xb72a3836 in WebCore::SharedTimerQt::timerEvent (this=0x978f1b0, ev=0xbffdf02c) at ../../../../WebCore/platform/qt/SharedTimerQt.cpp:105
#12 0xb4fc58c6 in QObject::event (this=0x978f1b0, e=0xbffdf02c) at kernel/qobject.cpp:1073
#13 0xb5245f37 in QApplicationPrivate::notify_helper (this=0x96ab860, receiver=0x978f1b0, e=0xbffdf02c) at kernel/qapplication.cpp:4057
#14 0xb52462ac in QApplication::notify (this=0xbffdf39c, receiver=0x978f1b0, e=0xbffdf02c) at kernel/qapplication.cpp:3604
#15 0xb4fadad4 in QCoreApplication::notifyInternal (this=0xbffdf39c, receiver=0x978f1b0, event=0xbffdf02c) at kernel/qcoreapplication.cpp:610
#16 0x0805166c in QCoreApplication::sendEvent (receiver=0x978f1b0, event=0xbffdf02c) at /usr/include/QtCore/qcoreapplication.h:213
#17 0xb4fe84a4 in QTimerInfoList::activateTimers (this=0x9692f34) at kernel/qeventdispatcher_unix.cpp:572
#18 0xb4fe5c6a in timerSourceDispatch (source=0x9692f00) at kernel/qeventdispatcher_glib.cpp:164
#19 0xb4766b88 in IA__g_main_context_dispatch (context=0x9692e80) at /build/buildd/glib2.0-2.20.1/glib/gmain.c:1814
#20 0xb476a0eb in g_main_context_iterate (context=0x9692e80, block=1, dispatch=1, self=0x9693b38) at /build/buildd/glib2.0-2.20.1/glib/gmain.c:2448
#21 0xb476a268 in IA__g_main_context_iteration (context=0x9692e80, may_block=1) at /build/buildd/glib2.0-2.20.1/glib/gmain.c:2511
#22 0xb4fe4d66 in QEventDispatcherGlib::processEvents (this=0x969f1b0, flags={i = -1073876556}) at kernel/qeventdispatcher_glib.cpp:324
#23 0xb5312174 in QGuiEventDispatcherGlib::processEvents (this=0x969f1b0, flags={i = -1073876508}) at kernel/qguieventdispatcher_glib.cpp:202
#24 0xb4faa0f0 in QEventLoop::processEvents (this=0xbffdf278, flags={i = -1073876436}) at kernel/qeventloop.cpp:149
#25 0xb4faa34b in QEventLoop::exec (this=0xbffdf278, flags={i = -1073876352}) at kernel/qeventloop.cpp:200
#26 0xb4fae429 in QCoreApplication::exec () at kernel/qcoreapplication.cpp:888
#27 0xb5245c50 in QApplication::exec () at kernel/qapplication.cpp:3526

[David Kilzer (:ddkilzer)]

(In reply to comment #21)
> > Sorry, I haven't had time and this hasn't been a priority.  I'll try to get to
> > it in the next week or so, but anyone else with a Windows PC and a Mac are
> > welcome to do the research and post their results here.  :)
> 
> david, anything I could help w/ with a linux box and qtwebkit build ?
> 
> we are facing a crash running this specific layouttest in qtwebkit (see below).
> I am not sure if on Mac it was crashing or just "failing"  though ...
> 
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0xb469a700 (LWP 23201)]
> 0xb6d610c8 in WebCore::Node::childNeedsStyleRecalc (this=0x8) at
> ../../../../WebCore/dom/Node.h:270
> 270        bool childNeedsStyleRecalc() const { return m_childNeedsStyleRecalc;
> }

On the Mac, this test timed out (would hang) because it kept loading over and over again.  I haven't tried re-enabling the test recently, so I don't know what the current behavior is.

I suspect you're running into a different issue, though, since the test never crashed on the Mac.

It appears that the "this" pointer is almost NULL (0x8) when Node::childNeedsStyleRecalc() is called, which is bad.  You might try running this single test under valgrind to see if you find any issues before it crashes (like some kind of memory smasher).

[David Kilzer (:ddkilzer)]

(In reply to comment #22)
> I suspect you're running into a different issue, though, since the test never
> crashed on the Mac.

Well, obviously it crashed at one point, hence the name of the test, but when I filed this bug it was not crashing.

You might also try to look at the original patch committed with this test to see how the original crash was fixed.

[Antonio Gomes]

> (In reply to comment #22)
> > I suspect you're running into a different issue, though, since the test never crashed on the Mac.
> Well, obviously it crashed at one point, hence the name of the test, but when I filed this bug it was not crashing.
> 
> You might also try to look at the original patch committed with this test to
> see how the original crash was fixed.

So I found the original commit of this test. See http://code.staikos.net/cgi-bin/gitweb.cgi?p=webkit;a=commit;h=460c62b8190c48d964755f694942045ed791100d

in this patch we have:

(...)
@@ -847,6 +847,9 @@ void Node::insertedIntoDocument()
void Node::removedFromDocument()
{
+    if (m_document && m_document->getCSSTarget() == this)
+        m_document->setCSSTarget(0);
+
    setInDocument(false);
    removedFromTree(false);
}
(...)

in ToT method looks like this though:

(...)
void Node::removedFromDocument()
{
    if (!eventListeners().isEmpty())
        document()->registerDisconnectedNodeWithEventListeners(this);

    setInDocument(false);
}
(...)

you can note that the "fix" in the former commit was removed in this commit http://code.staikos.net/cgi-bin/gitweb.cgi?p=webkit;a=commit;h=e8c3a2adfa7874c152f431d48423a104f974f170  

* dom/Node.cpp:
(WebCore::Node::insertedIntoDocument): Removed call to InsertedIntoTree, since it's only non-empty in subclasses of ContainerNode.
(WebCore::Node::removedFromDocument): Ditto. Also removed setCSSTarget. Since a CSS target has to be an Element, this can be moved down to ContainerNode (or it could be moved down to Element for that matter).

If I re-add 

+    if (m_document && m_document->getCSSTarget() == this)
+        m_document->setCSSTarget(0);

all goes fine: test succeeds in both release and debug builds.

@david what do you suggest?

[David Kilzer (:ddkilzer)]

(In reply to comment #24)
> If I re-add 
> 
> +    if (m_document && m_document->getCSSTarget() == this)
> +        m_document->setCSSTarget(0);
> 
> all goes fine: test succeeds in both release and debug builds.
> 
> @david what do you suggest?

Please post a patch with a "review?" flag that adds the above code back.  This test now crashes on the Mac as well when run individually.  :(

$ ./WebKitTools/Scripts/run-webkit-tests --debug LayoutTests/fast/dom/cssTarget-crash.html
[...]
Running tests from /Volumes/Data/WebKit.git/LayoutTests
Testing 1 test cases.
fast/dom .
fast/dom/cssTarget-crash.html -> crashed

35.30s total testing time

1 test case (100%) crashed

ddkilzer-- for not re-enabling this test sooner.

[Antonio Gomes]

Created attachment 32285 [details]
patch 0.2

Re-added original fix the cssTarget-crash.

it fixes running this test in QtWebKit for both release and debug modes. Should fix mac (and make it possible to unskip it).

[Darin Adler]

Comment on attachment 32285 [details]
patch 0.2

http://trac.webkit.org/changeset/40475 moved the setCSSTarget call from Node::removedFromDocument into ContainerNode::removedFromDocument. But that wasn't when this bug was introduced.

It was introduced by a bad merge back in http://trac.webkit.org/changeset/40499. The right fix is to add the code back, but to add it back to ContainerNode::removedFromDocument, not Node::removedFromDocument.

I'll take care of this.

[Darin Adler]

Comment on attachment 32285 [details]
patch 0.2

Clearing the review flag since I landed the fix as http://trac.webkit.org/changeset/45557 but not closing the bug, because the test still fails. The crash is gone, but now the test doesn't terminate. That's presumably because of loader changes that happened while it was disabled.

[David Kilzer (:ddkilzer)]

(In reply to comment #28)
> (From update of attachment 32285 [details])
> Clearing the review flag since I landed the fix as
> http://trac.webkit.org/changeset/45557 but not closing the bug, because the
> test still fails. The crash is gone, but now the test doesn't terminate. That's
> presumably because of loader changes that happened while it was disabled.

No, the test never terminating was why this bug was filed in the first place.

[Darin Adler]

(In reply to comment #29)
> No, the test never terminating was why this bug was filed in the first place.

In that case, we probably want to change the title back to remove the particular version from the title and perhaps even assign back to ddkilzer (if you still plan to work on it).

[David Kilzer (:ddkilzer)]

See also Bug 26886.

[Erik Arvidsson]

I have a modified version of the layout test that terminates under both circumstances.

However, it would be good if we could decide what behavior we want. Do we want the form to get submitted to the server like IE and Opera or do we want to stay on the current page without a submit like Firefox?

My vote is for the form to get submitted (current behavior).

I can also add another layout test that ensures that behavior if that is the behavior we want.

[Darin Adler]

(In reply to comment #32)
> I have a modified version of the layout test that terminates under both
> circumstances.
> 
> However, it would be good if we could decide what behavior we want. Do we want
> the form to get submitted to the server like IE and Opera or do we want to stay
> on the current page without a submit like Firefox?
> 
> My vote is for the form to get submitted (current behavior).

As I understand it, the issue is: When you submit a form from JavaScript, is the decision of what URL to submit to at the time you call submit() or decided later on when the actual submission is done? Is that the right question?

You say that current behavior matches IE and Opera, but not Firefox. Does it also match Safari 4? Does HTML 5 have anything to say about this?

> I can also add another layout test that ensures that behavior if that is the
> behavior we want.

We definitely want that no matter what!

[David Kilzer (:ddkilzer)]

Resetting to the default assignee as I haven't had time to look into this thoroughly.

[Erik Arvidsson]

(In reply to comment #33)
> (In reply to comment #32)
> > I have a modified version of the layout test that terminates under both
> > circumstances.
> > 
> > However, it would be good if we could decide what behavior we want. Do we want
> > the form to get submitted to the server like IE and Opera or do we want to stay
> > on the current page without a submit like Firefox?
> > 
> > My vote is for the form to get submitted (current behavior).
> 
> As I understand it, the issue is: When you submit a form from JavaScript, is
> the decision of what URL to submit to at the time you call submit() or decided
> later on when the actual submission is done? Is that the right question?

There are two orthogonal issues here:

1. In Firefox, if there is no form data, the method is GET and the action is a named element on the page (eg. #a) submitting the form does not leave the page. It adds sets the hash to #a. If there is form data or there is no matching named element on the page the form submit works as usual.

2. Changing the action after submit is called should not change the action of the in flight submit.

> 
> You say that current behavior matches IE and Opera, but not Firefox. Does it
> also match Safari 4? Does HTML 5 have anything to say about this?

The current behavior of 1 matches IE, Opera and Safari 4. HTML5 says the following:

Step 15 at http://dev.w3.org/html5/spec/Overview.html#concept-form-submit says to do a Mutate action. Mutate action says to do a Navigate, http://dev.w3.org/html5/spec/Overview.html#navigate. Point 4 of that says:

"Fragment identifiers: If the absolute URL of the new resource is the same as the address of the active document of the browsing context being navigated, ignoring any <fragment> components of those URLs, and the new resource is to be fetched using HTTP GET or equivalent, and the absolute URL of the new resource has a <fragment> component (even if it is empty), then navigate to that fragment identifier and abort these steps."

"abort these steps" means that we should not fetch the resource and therefore it seems like we are NOT doing the right thing in case 1.



The current behavior of 2 matches HTML5. The following happens when submit() is called http://dev.w3.org/html5/spec/Overview.html#concept-form-submit and it states in point 8 that it should use the action of the form when submitting, not the action of the form when the js thread is done. 

> 
> > I can also add another layout test that ensures that behavior if that is the
> > behavior we want.
> 
> We definitely want that no matter what!


Conclusion: 

1. We need to fix the case where a GET form is submitted to the same URL + fragment identifier.
2. The current layout test should pass and never time out
3. Add a layout test that tests that we submit to the current action when submit() was called and later changes should not change that. This can be done independently from the other fixes.
4. Add a new clearer layout test that tests that submitting an empty form to the same url + fragment identifier does not replace the current page.

[Yael]

(In reply to comment #35)
> Conclusion: 
> 
> 1. We need to fix the case where a GET form is submitted to the same URL + fragment identifier.
> 2. The current layout test should pass and never time out
> 3. Add a layout test that tests that we submit to the current action when submit() was called and later changes should not change that. This can be done independently from the other fixes.
> 4. Add a new clearer layout test that tests that submitting an empty form to the same url + fragment identifier does not replace the current page.

I agree with your analysis with one exception. This behavior has nothing to do with the form being empty or not. It is about checking if the action URL and the location URL match or not. If the form is empty, there is no query string, thus it is more likely that you would get matching URLs when running tests.

[Yael]

Created attachment 77524 [details]
Patch

Do not reload the page when submitting a form using "GET" method and the form action url matches the location url, except for the fragment.
This matches the behavior of Firefox and Opera, but not IE. It also matches the HTML5 spec, as described above in comment #35.

[Adam Barth]

Comment on attachment 77524 [details]
Patch

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

This bug as a somewhat complicated history, but from my reading of the history, this patch is what we want.

> LayoutTests/fast/forms/submit-change-fragment.html:12
> +    setTimeout("continueTest();", 20);

Can we use onHashChange instead of setTimeout?

[Yael]

(In reply to comment #38)
> (From update of attachment 77524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77524&action=review
> 
> This bug as a somewhat complicated history, but from my reading of the history, this patch is what we want.
> 
> > LayoutTests/fast/forms/submit-change-fragment.html:12
> > +    setTimeout("continueTest();", 20);
> 
> Can we use onHashChange instead of setTimeout?

Thank you for the review. I just updated the test to use onhashchange.

[Yael]

Committed r74801 <http://trac.webkit.org/changeset/74801>.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74801 might have broken Leopard Intel Debug (Tests)

[Yael]

(In reply to comment #41)
> http://trac.webkit.org/changeset/74801 might have broken Leopard Intel Debug (Tests)

I doubt that.
The same exact set of failures is also in <http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Tests%29/builds/25253/steps/layout-test/logs/stdio> , (build results for http://trac.webkit.org/changeset/74796).

[Mahesh Kulkarni]

*** Bug 22590 has been marked as a duplicate of this bug. ***