Bug 131044 – REGRESSION(r166395): It made all media tests crash on EFL

• Component: Media
• Status: REOPENED

[Csaba Osztrogonác]

before: http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK1/builds/5723
after: http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK1/builds/5724

crash log:

STDERR: 1   0x7fa0359401c0
STDERR: 2   0x7fa030671ff0
STDERR: 3   0x7fa033b1de27 WebCore::HTMLElement::ieForbidsInsertHTML() const
STDERR: 4   0x7fa033b209ad WebCore::HTMLElement::setInnerText(WTF::String const&, int&)
STDERR: 5   0x7fa033b76d8b WebCore::HTMLTextFormControlElement::setInnerTextValue(WTF::String const&)
STDERR: 6   0x7fa033b9125c WebCore::TextFieldInputType::updateInnerTextValue()
STDERR: 7   0x7fa0339c10f9 WebCore::Element::attributeChanged(WebCore::QualifiedName const&, WTF::AtomicString const&, WTF::AtomicString const&, WebCore::Element::AttributeModificationReason)
STDERR: 8   0x7fa0339bf4fc WebCore::Element::didAddAttribute(WebCore::QualifiedName const&, WTF::AtomicString const&)
STDERR: 9   0x7fa0339c1d7d WebCore::Element::addAttributeInternal(WebCore::QualifiedName const&, WTF::AtomicString const&, WebCore::Element::SynchronizationOfLazyAttribute)
STDERR: 10  0x7fa0339c3dad WebCore::Element::setAttributeWithoutSynchronization(WebCore::QualifiedName const&, WTF::AtomicString const&)
STDERR: 11  0x7fa033c1b6cb WebCore::MediaControlRewindButtonElement::create(WebCore::Document&)
STDERR: 12  0x7fa03436128e WebCore::MediaControlsApple::createControls(WebCore::Document&)
STDERR: 13  0x7fa034361b65 WebCore::MediaControls::create(WebCore::Document&)
STDERR: 14  0x7fa033b422e4 WebCore::HTMLMediaElement::createMediaControls()
STDERR: 15  0x7fa033b424d8 WebCore::HTMLMediaElement::configureMediaControls()
STDERR: 16  0x7fa033b4dde4 WebCore::HTMLMediaElement::insertedInto(WebCore::ContainerNode&)
STDERR: 17  0x7fa0339842ad WebCore::ContainerNode::parserAppendChild(WTF::PassRefPtr<WebCore::Node>)
STDERR: 18  0x7fa033bcd9e9 WebCore::HTMLConstructionSite::executeQueuedTasks()
STDERR: 19  0x7fa033bd7594 WebCore::HTMLDocumentParser::constructTreeFromHTMLToken(WebCore::HTMLToken&)
STDERR: 20  0x7fa033bd7bf4 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)
STDERR: 21  0x7fa033bd88d2 WebCore::HTMLDocumentParser::append(WTF::PassRefPtr<WTF::StringImpl>)
STDERR: 22  0x7fa03398ee2e WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter&)
STDERR: 23  0x7fa033ccfe03 WebCore::DocumentWriter::end()
STDERR: 24  0x7fa033cc5e07 WebCore::DocumentLoader::finishedLoading(double)
STDERR: 25  0x7fa033d46841 WebCore::CachedResource::checkNotify()
STDERR: 26  0x7fa033d449c0 WebCore::CachedRawResource::finishLoading(WebCore::ResourceBuffer*)
STDERR: 27  0x7fa033d0db1e WebCore::SubresourceLoader::didFinishLoading(double)
STDERR: 28  0x7fa0343ad371
STDERR: 29  0x7fa02e55704a
STDERR: 30  0x7fa02e5765db
STDERR: 31  0x7fa02e5765f9


Mark it as blocker/P1, because EFL bots exit early because of this regression, so they have near 0 test coverage since 5 days / 200 revisions.
The bug should be fixed as soon as possible or media tests should be skipped if you are interested in catching new regressions by the bots.

[Csaba Osztrogonác]

Created attachment 228389 [details]
backtrace

Here is a gdb backtrace for media/audio-concurrent-supported.html,
it might help someone interested in fixing this regression.

My question is why EFL uses MediaControlsApple.cpp?

[Ryuan Choi]

(In reply to comment #1)
> Created an attachment (id=228389) [details]
> backtrace
> 
> Here is a gdb backtrace for media/audio-concurrent-supported.html,
> it might help someone interested in fixing this regression.

More information,
This crash is started from r166395 ( Bug 130882 ).
But I am not sure why it occurs in EFL because it is general code.

> 
> My question is why EFL uses MediaControlsApple.cpp?

EFL may need to have own MediaControls but it looks not related to this issue.

[Adrien Destugues]

We have the same problem in the Haiku port (also using MediaControlsApple.cpp). Moving the setPseudo call to the create methods, after the call to setType, seems to work around this, but I don't know if that's the proper fix.

[Adrien Destugues]

Here is our patch for this: https://github.com/haiku/webkit/commit/1a7810025da941981a6e39cc4b199eb47615e0c2

[Ryuan Choi]

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=228389) [details] [details]
> > backtrace
> > 
> > Here is a gdb backtrace for media/audio-concurrent-supported.html,
> > it might help someone interested in fixing this regression.
> 
> More information,
> This crash is started from r166395 ( Bug 130882 ).
> But I am not sure why it occurs in EFL because it is general code.

At least, I realized that GTK and (maybe) MAC does not use this crash path because they use MEDIA_CONTROLS_SCRIPT.

Just for test, I tried to use the code of GTK port.
And media/audio-concurrent-supported.html is passed.
(control is bit ugly for efl port)

IMO, we should turn on the MEDIA_CONTROLS_SCRIPT. but I am not sure whether it is correct solution for this crash.

Anyway, I will prepare proper css/js for EFL port. (It may takes more time)

[Csaba Osztrogonác]

It seems the EFL bots are still broken from this regression.
(during ~500 revisions and 11 days)

Is there any plan to fix them to be able catch new regressions again?

[Gyuyoung Kim]

(In reply to comment #6)
> It seems the EFL bots are still broken from this regression.
> (during ~500 revisions and 11 days)
> 
> Is there any plan to fix them to be able catch new regressions again?

Yes, EFL bots have broken too long time. I couldn't take a look this problem due to my various works. Let me try to fix it as soon as possible.

[Gyuyoung Kim]

Almost media tests are being passed on EFL buildbot. However, unfortunately I failed to find what revision fixed this issue. To maintain those test on EFL port again, I unskip it by r173864.

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

[Csaba Osztrogonác]

(In reply to comment #8)
> Almost media tests are being passed on EFL buildbot. However, unfortunately I failed to find what revision fixed this issue. To maintain those test on EFL port again, I unskip it by r173864.
> 
> http://trac.webkit.org/changeset/173864

Not at all, all media tests still fail on the EFL bot and this
unskipping made the runtime from 22mins to 3.5hours and the
number of failing test from 30-40 to 300+. :-/

[Gyuyoung Kim]

(In reply to comment #9)
> (In reply to comment #8)
> > Almost media tests are being passed on EFL buildbot. However, unfortunately I failed to find what revision fixed this issue. To maintain those test on EFL port again, I unskip it by r173864.
> > 
> > http://trac.webkit.org/changeset/173864
> 
> Not at all, all media tests still fail on the EFL bot and this
> unskipping made the runtime from 22mins to 3.5hours and the
> number of failing test from 30-40 to 300+. :-/

Ossy, something is weird. When I test it locally, there were dozens of failing tests. Let me check it again.

[Adrien Destugues]

Created attachment 239541 [details]
Patch

[Darin Adler]

Brent, Eric, Jer, could one of you review this?

One thing I am noticing is that if the setPseudo calls are now moved after the elements are completely constructed, we could possibly go back to calling shadowPseudoId() rather than separately hardcoding each ID. I suspect the reason that didn’t work before is that we were calling it during construction.

[Brent Fulgham]

I think it would be helpful if someone would state what the fix is meant to do.

It looks like the issue is that the 'setPseudo' call in the constructor causes an error when the m_innerText of a TextFieldInputType class is accessed. Apparently this does not yet exist during construction time.

It seems strange to have to set these attributes of the class after construction, but I guess it's no worse than the existing need to call 'setType' or 'ensureUserAgentShadowRoot' after the object is constructed.

This seems like clunky design, but I'm not sure how to address it at this point.

[Jer Noble]

(In reply to comment #13)
> I think it would be helpful if someone would state what the fix is meant to do.
> 
> It looks like the issue is that the 'setPseudo' call in the constructor causes an error when the m_innerText of a TextFieldInputType class is accessed. Apparently this does not yet exist during construction time.

Yeah, if you're calling a virtual method (Element::attributeChanged()) from a constructor you're going to have a bad time. (<meme>)

> It seems strange to have to set these attributes of the class after construction, but I guess it's no worse than the existing need to call 'setType' or 'ensureUserAgentShadowRoot' after the object is constructed.
> 
> This seems like clunky design, but I'm not sure how to address it at this point.

I don't think there's a better way.

[Jer Noble]

(In reply to comment #12)
> Brent, Eric, Jer, could one of you review this?
> 
> One thing I am noticing is that if the setPseudo calls are now moved after the elements are completely constructed, we could possibly go back to calling shadowPseudoId() rather than separately hardcoding each ID. I suspect the reason that didn’t work before is that we were calling it during construction.

Someone added a FIXME to shadowPseudoId() asking clients not to overload it.  So I don't think we should go back to using it instead.

[Benjamin Poulain]

Comment on attachment 239541 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        No new tests: fixes existing tests.
> +        The pseudo id cannot be set before the elements get a type.

The patch is not necessarily wrong, but the changelog is unsatisfactory.

Here you should explain exactly why the crash occur and why this is the right fix for it.

[Csaba Osztrogonác]

The media tests are still skipped. Is EFL port interested in fixing this old regression?

[Gyuyoung Kim]

(In reply to comment #17)
> The media tests are still skipped. Is EFL port interested in fixing this old
> regression?

Now media tests are passed on EFL port, except for 25 tests. I update this result in r182657 - http://trac.webkit.org/changeset/182657.