Bug 42592 – PopupMenu refactoring in preparation to WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Luiz Agostini]

PopupMenu refactoring in preparation to WebKit2

[Sam Weinig]

Before you start working on this, can you briefly explain how you are planning on tackling this?

[Luiz Agostini]

Created attachment 62004 [details]
patch 1

[Luiz Agostini]

(In reply to comment #1)
> Before you start working on this, can you briefly explain how you are planning on tackling this?

The first patches will be just poof of concept to try to receive feedback.

I explained my objectives and what I have made so far in the change log of the patch.

[WebKit Review Bot]

Attachment 62004 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3578244

[WebKit Review Bot]

Attachment 62004 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3609033

[WebKit Review Bot]

Attachment 62004 [details] did not build on win:
Build output: http://queues.webkit.org/results/3348908

[Darin Fisher (:fishd, Google)]

This patch breaks the Chromium port.  Please don't commit this.

It seems wrong to delete PopupMenu.h.  If the goal is to provide an indirect way to request a popup menu, then interfaces should probably be added to do so, but the concrete implementation should remain (and be adapted to the interfaces).

[Luiz Agostini]

(In reply to comment #7)
> This patch breaks the Chromium port.  Please don't commit this.

Sure I will not commit anything before I have a patch that does not break any platform. I submitted this patch just to show which are my plans and to try to get some feedback. My main questions are:

1 - Are the interfaces declared in SelectMethodClient.h ok? They will replace  PopupMenu and SearchPopupMenu.
2 - Is it ok for Chrome/ChromeClient to be responsible for providing the popup menu factory?
3 - Is it ok for all the platforms to move popup menu implementations to WebKit layer?

> It seems wrong to delete PopupMenu.h.  If the goal is to provide an indirect way to request a popup menu, then interfaces should probably be added to do so, but the concrete implementation should remain (and be adapted to the interfaces).

PopupMenu.h is used by all the platforms and is full of preprocessor macros for each platform. I think that it could be removed and that each platform could have its own header containing just platform's fields, with no #if PLATFORM() in it. This is the first step.

If each platform have its own independent implementation, then it seems natural to me for Chrome/ChromeClient to be responsible for providing the popups. And it seems natural to move popups implementation to WebKit layer too.

[Luiz Agostini]

Darin Adler, do you have any input on this approach?

[Darin Adler]

First thought: To me it seems overkill to have this new factory class. The ChromeClient already contains this type of function. Having a separate factory just for the pop-ups seems unnecessary.

[Darin Adler]

There are two different ways we intend to do platform-specific work.

One is to make a platform independence layer inside the platform directory. The other is to use clients and delegate the work to platform-specific WebKit code.

When the requirement is simple enough we like to stick with the platform directory approach. However, a bad thing to avoid is to put the code in the platform directory, but then break the abstraction layer and effectively delegate to WebKit anyway, doing it "at the bottom" inside the platform directory code.

It seems OK to move to a client approach for pop-up menus if we can't practically do this with the platform directory layer. It does seem possible it would cause us to lose out on code sharing for cases where it could work. For example, if this decreases the ability for the Mac OS X port to share with the Chromium Mac port, that would be too bad.

It is nice when we can make platform independence something simple WebCore can handle without shipping everything over to the WebKit layer. It's too bad this case is no longer simple enough to do in the more straightforward way.

[Darin Fisher (:fishd, Google)]

In case this isn't know, the way we handle popups in Chromium is as follows.  On Windows/Linux, a popup is actually rendered by WebCore (see platform/chromium/PopupMenuChromium.cpp).  All we need the WebKit layer to do is to provide us with a container for a ScrollView.  (We actually use a subclass of ScrollView called FramelessScrollView.)  That allows the UI process to position the popup window for us (it handles the HWND), and WebCore is given a ScrollView to render the popup menu contents into.  This works really well!

Now, on Mac, we have a different setup.  There we actually want to render the popup menu using Cocoa.  The call to the WebKit layer is in that case given all of the parameters needed to populate the popup menu, and it goes about rendering it in the UI process.

I would like us to find a solution that supports both models, just like we have done for Chromium.  I think it is nice to leverage WebCore when possible, but provide the option of rendering in a platform specific way.

One final note.  Even on OSX, we still let PopupMenuChromium do the rendering in cases where we do not want a Cocoa popup menu.  This happens for things like password autocomplete.

[Kenneth Rohde Christiansen]

Just to explain why Luiz is working on this.

We cannot use the Chromium approach for Qt and WebKit2 as we wants to reimplement the UI on the UI side when targeting mobile devices which need special UI for select inputs, just like on the iPhone. Currently we [QtWebKit] have a platform plugin that each platform can implement we they want to override the default UI (based on QCombobox).

[Darin Fisher (:fishd, Google)]

OK, so how about if we define an interface for PopupMenu, and then let ChromeClient return a PopupMenu.  Then, allow for there to be implementations of PopupMenu in WebCore/platform just as we have today.  This also allows for the WebKit layer to provide its own implementation.  This should be a lot easier to implement since it will involve less disruption of other ports.

[Luiz Agostini]

Created attachment 62449 [details]
patch 2

[Eric Seidel (no email)]

Attachment 62449 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3617050

[Luiz Agostini]

Darin Fisher, what do you think about this latest patch?

[Luiz Agostini]

Created attachment 62455 [details]
patch 3

mac build fix

[Luiz Agostini]

Created attachment 62461 [details]
patch 4

efl

[Luiz Agostini]

Created attachment 62484 [details]
rebase

rebase

[Luiz Agostini]

Created attachment 62570 [details]
rebase

[Darin Fisher (:fishd, Google)]

Comment on attachment 62570 [details]
rebase

WebCore/page/ChromeClient.h:132
 +          virtual PassRefPtr<SelectUIClient> createSelectUIClient(PopupMenuClient*) const = 0;
I think it would be better to name these methods:

  createSelectPopupMenu
  createSearchPopupMenu

And, these should each take a PopupMenuClient* and return a PassRefPtr<PopupMenu>.
PopupMenu should be made an abstract interface.

If you look at PopupMenu.h today, it is a mess of #ifdefs.  There is very little
that is shared between the ports other than the interface itself.  So, making
PopupMenu be an interface makes sense.  The concrete class should be renamed:
PopupMenuWin, PopupMenuChromium, etc.

Doing so has a couple other nice properties:

 1- It avoids introducing new terminology ("what is a select ui client?")
 2- It avoids a dependency from WebCore/platform on WebCore/page

[Luiz Agostini]

(In reply to comment #22)
> (From update of attachment 62570 [details])
> WebCore/page/ChromeClient.h:132
>  +          virtual PassRefPtr<SelectUIClient> createSelectUIClient(PopupMenuClient*) const = 0;
> I think it would be better to name these methods:
> 
>   createSelectPopupMenu
>   createSearchPopupMenu

Ok. I prefer these names as well.

> 
> And, these should each take a PopupMenuClient* and return a PassRefPtr<PopupMenu>.

Should PopupMenu implement all methods form SearchPopupMenu? Today they are different classes in webkit.

I would prefer SearchPopupMenu not to inherit from PopupMenu. The reason is that it would not be simple to inherit them in platforms without having an ambiguity in PopupMenu. This is why I chose not to use inheritance:

         PopupMenu   <----------   MyPopupMenu
                   ^                                                    ^
                   |                                                     | (this arrow would be a problem)
                   |                                                     |
  SearchPopupMenu  <------- MySearchPopupMenu

I do prefer not to use inheritance in this case. What do you think?

> PopupMenu should be made an abstract interface.

That is what I have done but the names I chose were not so good (SelectUIClient, SearchUIClient). What do you think about AbstractPopupMenu and AbstractSearchPopupMenu. Or maybe PopupMenuInterface and SearchPopupMenuInterface.

> 
> If you look at PopupMenu.h today, it is a mess of #ifdefs.  There is very little
> that is shared between the ports other than the interface itself.  So, making
> PopupMenu be an interface makes sense.  The concrete class should be renamed:
> PopupMenuWin, PopupMenuChromium, etc.

The whole idea is to create a PopupMenu interface. At first I was thinking in creating headers for each platform but after first comments in this bug I found it too disruptive for the platforms. To avoid big changes in PopupMenu.h I created an interface (SelectUIClient) and made PopupMenu inherit form it. Each platform may then remove its implementation from PopupMenu.h later if needed and/or wanted.

What if I keep this approach and find a better name for the interface?

> 
> Doing so has a couple other nice properties:
> 
>  1- It avoids introducing new terminology ("what is a select ui client?")

SelectUIClient was not a good option. The choice is based on the fact that it may be not a popup. But I do agree the using popup in the name makes it more clear.

>  2- It avoids a dependency from WebCore/platform on WebCore/page

Is keeping the interfaces definitions in platform ok?

[Darin Fisher (:fishd, Google)]

(In reply to comment #23)
...
> > And, these should each take a PopupMenuClient* and return a PassRefPtr<PopupMenu>.
> 
> Should PopupMenu implement all methods form SearchPopupMenu? Today they are different classes in webkit.
> 
> I would prefer SearchPopupMenu not to inherit from PopupMenu. The reason is that it would not be simple to inherit them in platforms without having an ambiguity in PopupMenu. This is why I chose not to use inheritance:
> 
>          PopupMenu   <----------   MyPopupMenu
>                    ^                                                    ^
>                    |                                                     | (this arrow would be a problem)
>                    |                                                     |
>   SearchPopupMenu  <------- MySearchPopupMenu
> 
> I do prefer not to use inheritance in this case. What do you think?

Virtual inheritance in C++ exists to support this use case, although it is
usually something that people avoid.

It looks like you used composition to solve this problem in SearchUIClient.
Perhaps the answer is to change SearchPopupMenu to have a popupMenu method
that returns a PopupMenu?


> > If you look at PopupMenu.h today, it is a mess of #ifdefs.  There is very little
> > that is shared between the ports other than the interface itself.  So, making
> > PopupMenu be an interface makes sense.  The concrete class should be renamed:
> > PopupMenuWin, PopupMenuChromium, etc.
> 
> The whole idea is to create a PopupMenu interface. At first I was thinking in creating headers for each platform but after first comments in this bug I found it too disruptive for the platforms. To avoid big changes in PopupMenu.h I created an interface (SelectUIClient) and made PopupMenu inherit form it. Each platform may then remove its implementation from PopupMenu.h later if needed and/or wanted.
> 
> What if I keep this approach and find a better name for the interface?

I think it is just a bunch of "svn mv" commands followed by renaming
the concrete class name.  I agree that it is tedious work, but it is
the right thing to do.


> >  2- It avoids a dependency from WebCore/platform on WebCore/page
> 
> Is keeping the interfaces definitions in platform ok?

Yes, definitely.  Sorry, I should have made this clear.

[Luiz Agostini]

Created attachment 62635 [details]
just a test. please do not review.

[Jeremy Orlow]

Comment on attachment 62635 [details]
just a test. please do not review.

Please don't set the review flag on stuff you don't want reviewed.

[Kenneth Rohde Christiansen]

(In reply to comment #26)
> (From update of attachment 62635 [details])
> Please don't set the review flag on stuff you don't want reviewed.

I believe Luiz set the flag to find out whether his patch compiles on the different platforms as he doesn't have access to all. Unfortunately, there seem to be no other way of doing this right now.

[Jeremy Orlow]

Oh.  Ugh.  There has to be a better way.

eric ^^^^^^

[Luiz Agostini]

Created attachment 62723 [details]
patch 5

[Eric Seidel (no email)]

Attachment 62723 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3635040

[Luiz Agostini]

Created attachment 62730 [details]
patch 6

rebase. mac build fix.

[Luiz Agostini]

Created attachment 62735 [details]
patch 7

rebase

[Eric Seidel (no email)]

Attachment 62735 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3605443

[Luiz Agostini]

Created attachment 62774 [details]
patch 8

[Luiz Agostini]

Created attachment 62776 [details]
patch 9

rebase

[Kenneth Rohde Christiansen]

78 // bool PopupMenuBrew::itemWritingDirectionIsNatural()
 79 // {
 80 //     return true;
 81 // }

This seems like an error.

[Luiz Agostini]

(In reply to comment #36)
> 78 // bool PopupMenuBrew::itemWritingDirectionIsNatural()
>  79 // {
>  80 //     return true;
>  81 // }
> 
> This seems like an error.

This method is now in ChromeClient. In all other platforms I have updated the platform's ChromeClient, but there is no concrete ChromeClient inherited class for brew in repository.  I kept this method commented out just to prevent loosing the information of what must be the return value for brew.

[Luiz Agostini]

Darin Adler, do you have any comments?

[Darin Fisher (:fishd, Google)]

Comment on attachment 62776 [details]
patch 9

Looks great overall.

Comments:

Are you sure you wouldn't want to make it valid for create{Search}PopupMenu
to return NULL?  Then you wouldn't need Empty{Search}PopupMenu.  It seems
reasonable for a Client to not create something.


>+++ b/WebCore/page/Chrome.cpp
...
>+PassRefPtr<SearchPopupMenu> Chrome::createSearchPopupMenu(PopupMenuClient* client) const
>+{
>+    return m_client->createSearchPopupMenu(client);
>+}
>+
> 
> } // namespace WebCore

nit: ^^^ only one new line before the close of the namespace


>diff --git a/WebCore/platform/brew/PopupMenuBrew.cpp b/WebCore/platform/brew/PopupMenuBrew.cpp
...
>+// bool PopupMenuBrew::itemWritingDirectionIsNatural()
>+// {
>+//     return true;
>+// }

^^^ ordinarily bad form to leave code commented out without an explanation.  or
perhaps you intended to delete this?


>diff --git a/WebCore/platform/brew/PopupMenuBrew.h b/WebCore/platform/brew/PopupMenuBrew.h
...
>+class PopupMenuBrew : public PopupMenu {
...
>+private:
>+    PopupMenuClient* m_popupClient;
>+    FrameView* m_view;
>+
>+    PopupMenuClient* client() const { return m_popupClient; }
>+
>+};

nit: ^^^ no new line before the "};"


>+++ b/WebCore/platform/chromium/PopupMenuChromium.h
...
>+class PopupMenuChromium : public PopupMenu {
>+public:
>+    PopupMenuChromium(PopupMenuClient*);
>+    ~PopupMenuChromium();
>+
>+    virtual void show(const IntRect&, FrameView*, int index);
>+    virtual void hide();
>+    virtual void updateFromElement();
>+    virtual void disconnectClient();
>+
>+private:
>+    PopupMenuClient* m_popupClient;
>+    PopupMenuPrivate p;
>+
>+    PopupMenuClient* client() const { return m_popupClient; }
>+};

PopupMenuPrivate is now unnecessary (it can be folded into PopupMenuChromium),
but that refactoring can happen in a separate patch, and you don't need to be
on the hook for it.


>+++ b/WebCore/platform/efl/PopupMenuEfl.h
...
>+private:
>+    PopupMenuClient* m_popupClient;
>+    FrameView* m_view;
>+
>+    PopupMenuClient* client() const { return m_popupClient; }
>+
>+};

nit: ^^^ no new line before "};"

Also, I think it is generally better form to list methods before
variables in the private section of a class.  This applies to the
other newly added classes in this patch.


>+++ b/WebCore/platform/haiku/PopupMenuHaiku.h
...
>+private:
>+    PopupMenuClient* m_popupClient;
>+    HaikuPopup* m_menu;
>+
>+    PopupMenuClient* client() const { return m_popupClient; }
>+
>+};

nit: ^^^ no new line before "};"


>+++ b/WebCore/platform/wx/PopupMenuWx.h
...
>+    PopupMenuClient* client() const { return m_popupClient; }
>+
>+};

nit: ^^^ no new line before "};"


>+++ b/WebKit/chromium/src/ChromeClientImpl.cpp

>+bool ChromeClientImpl::selectItemWritingDirectionIsNatural()
>+{
>+    return false;
>+}
>+
>+PassRefPtr<WebCore::PopupMenu> ChromeClientImpl::createPopupMenu(WebCore::PopupMenuClient* client) const
>+{
>+    return adoptRef(new WebCore::PopupMenuChromium(client));
>+}
>+
>+PassRefPtr<WebCore::SearchPopupMenu> ChromeClientImpl::createSearchPopupMenu(WebCore::PopupMenuClient* client) const
>+{
>+    return adoptRef(new WebCore::SearchPopupMenuChromium(client));
>+}

nit: ^^^ no need for the WebCore:: prefix in this file thanks to
the 'using namespace WebCore' at the top of the file.


>+++ b/WebKit2/WebKit2.xcodeproj/project.pbxproj
...
>@@ -1493,7 +1507,12 @@
> 				E1EE55F811F8F1BC00CCBEE4 /* WKBundleRange.cpp in Sources */,
> 				E1EE565611F8F71700CCBEE4 /* WKBundleNode.cpp in Sources */,
> 				1AE4976911FF658E0048B464 /* NPJSObject.cpp in Sources */,
>+<<<<<<< HEAD:WebKit2/WebKit2.xcodeproj/project.pbxproj
> 				1AE4987911FF7FAA0048B464 /* JSNPObject.cpp in Sources */,
>+=======
>+				D3B9484611FF4B6500032B39 /* WebPopupMenu.cpp in Sources */,
>+				D3B9484811FF4B6500032B39 /* WebSearchPopupMenu.cpp in Sources */,
>+>>>>>>> PopupMenu refactoring in preparation to WebKit2:WebKit2/WebKit2.xcodeproj/project.pbxproj

^^^ oops!

[Luiz Agostini]

Created attachment 63084 [details]
patch 10

[Darin Fisher (:fishd, Google)]

Comment on attachment 63084 [details]
patch 10

r=me, but would be nice to hear your thoughts to my first question.

[Luiz Agostini]

(In reply to comment #39)
> (From update of attachment 62776 [details])
> Looks great overall.
> 
> Comments:
> 
> Are you sure you wouldn't want to make it valid for create{Search}PopupMenu
> to return NULL?  Then you wouldn't need Empty{Search}PopupMenu.  It seems
> reasonable for a Client to not create something.

Sorry. I should have commented it before.

I am trying to keep RenderMenuList and RenderTextControlSingleLine as much as they were to avoid increasing even more the scope of this patch. I believe that to change those classes to accept NULL clients is a sensitive change. I prefer to avoid it for now.

I may work on it if you think it is needed but I would prefer to do it in future patches.

Thanks a lot for your comments and for reviewing such a big patch. :)

[Darin Fisher (:fishd, Google)]

(In reply to comment #42)
> I am trying to keep RenderMenuList and RenderTextControlSingleLine as much as they were to avoid increasing even more the scope of this patch. I believe that to change those classes to accept NULL clients is a sensitive change. I prefer to avoid it for now.
> 
> I may work on it if you think it is needed but I would prefer to do it in future patches.

Makes sense.  I think it is worth some thought (after this patch lands).  You might find that it makes for a cleaner solution, or maybe not.  Maybe the null checks required in consumer code would be too unpleasant.

[Luiz Agostini]

Committed r64422: <http://trac.webkit.org/changeset/64422>

[Nikolas Zimmermann]

This broke compilation on several platforms, please take a look at the buildbot.

[WebKit Review Bot]

http://trac.webkit.org/changeset/64422 might have broken Chromium Win Release

[Luiz Agostini]

Rolled out. Preparing build fixes.

[Luiz Agostini]

Committed r64425: <http://trac.webkit.org/changeset/64425>

[Jeremy Orlow]

You missed an adoptRef here: http://trac.webkit.org/changeset/64425/trunk/WebKit/chromium/tests/PopupMenuTest.cpp

I rolled this (and a bunch of other) changes into Chromium and had a lot of weird errors that I can't reproduce locally on my machine.  The only one that gave me a strong clue as to what was happening:
	base::(anonymous namespace)::StackDumpSignalHandler() [0x815bb30]
	0x4001c420
	WTF::RefCounted<>::~RefCounted() [0x889b6a6]
	WebCore::PopupMenu::~PopupMenu() [0x889b904]
	WebCore::PopupMenuChromium::~PopupMenuChromium() [0x889735c]
	WebCore::SearchPopupMenuChromium::~SearchPopupMenuChromium() [0x889d2fe]
	WTF::RefCounted<>::deref() [0x854d301]
	WTF::derefIfNotNull<>() [0x8a28850]
	WTF::RefPtr<>::operator=() [0x8a288fe]
	WebCore::RenderTextControlSingleLine::~RenderTextControlSingleLine() [0x8a27b64]
	WebCore::RenderObject::arenaDelete() [0x89f01b6]
	WebCore::RenderObject::destroy() [0x89f03aa]
	WebCore::RenderBoxModelObject::destroy() [0x899a77f]
	WebCore::RenderBox::destroy() [0x89911cd]
	WebCore::RenderBlock::destroy() [0x8962846]
	WebCore::Node::detach() [0x864abdc]
	WebCore::ContainerNode::detach() [0x85eee79]
	WebCore::Element::detach() [0x862d9d1]
	WebCore::HTMLInputElement::detach() [0x8728657]
	WebCore::ContainerNode::detach() [0x85eee4f]
	WebCore::Element::detach() [0x862d9d1]
	WebCore::ContainerNode::detach() [0x85eee4f]
	WebCore::Element::detach() [0x862d9d1]
	WebCore::ContainerNode::detach() [0x85eee4f]
	WebCore::Element::detach() [0x862d9d1]
	WebCore::ContainerNode::detach() [0x85eee4f]
	WebCore::Document::detach() [0x85fd524]
	WebCore::Frame::setView() [0x8858a8e]
	WebKit::WebFrameImpl::createFrameView() [0x84ee1de]
	WebKit::FrameLoaderClientImpl::makeDocumentView() [0x8563386]
	WebKit::FrameLoaderClientImpl::transitionToCommittedForNewPage() [0x8563399]
	WebCore::FrameLoader::transitionToCommitted() [0x87f402b]
	WebCore::FrameLoader::commitProvisionalLoad() [0x87f456c]
	WebCore::DocumentLoader::commitIfReady() [0x87e0535]
	WebCore::DocumentLoader::commitLoad() [0x87e055b]
	WebCore::DocumentLoader::receivedData() [0x87e05e8]
	WebCore::FrameLoader::receivedData() [0x87ee2c3]
	WebCore::MainResourceLoader::addData() [0x8801dc4]
	WebCore::ResourceLoader::didReceiveData() [0x880c2fd]
	WebCore::MainResourceLoader::didReceiveData() [0x8801338]
	WebCore::ResourceLoader::didReceiveData() [0x880ba1c]
	WebCore::ResourceHandleInternal::didReceiveData() [0x94c32e9]
	webkit_glue::WebURLLoaderImpl::Context::OnReceivedData() [0x909ef01]
	(anonymous namespace)::RequestProxy::NotifyReceivedData() [0x90ca101]
	DispatchToMethod<>() [0x90c62e0]
	RunnableMethod<>::Run() [0x90c6316]
	MessageLoop::RunTask() [0x814aff9]
	MessageLoop::DeferOrRunPendingTask() [0x814b0a5]
	MessageLoop::DoWork() [0x814b335]
	base::MessagePumpForUI::HandleDispatch() [0x818da54]
	(anonymous namespace)::WorkSourceDispatch() [0x818daac]
	0x4067ecf6
	0x406820b3
	0x4068266e
	base::MessagePumpForUI::RunWithDispatcher() [0x818e03b]
	base::MessagePumpForUI::Run() [0x818e977]
	MessageLoop::RunInternal() [0x814bb67]
	MessageLoop::RunHandler() [0x814bb81]
	MessageLoop::Run() [0x814bc25]
	TestShell::WaitTestFinished() [0x8052d2d]
	TestShell::RunFileTest() [0x805306b]

I know that this patch was a major pain to land, but I think I'm going to need to speculatively roll it out.  I'll try to quickly confirm whether or not it was the actual cause of problems.

Sorry for the inconvenience!

[Jeremy Orlow]

I've got bad news.  It looks like my revert fixed the problems I was seeing on the bots, which means that this patch was indeed the culprit.  Before reverting, I skimmed through (which is how I saw the one memory leak I already pointed out), but I didn't see anything else.  The fact that this was only showing up as intermittent errors on only a couple of the bots (and I couldn't even reproduce it locally) means it's probably a race or memory issue of some sort.  Is there any weak reference/pointer in your code that might be getting accessed after the memory is freed or something?

I really wish I could point you towards more info, but unfortunately that roll is already off the console view of Chromium's waterfall, so I can only access it from work tomorrow.  I know that it won't be a whole lot of info to go on though.  And unfortunately there's no good way to try a change on the full battery of Chromium bots without committing to WebKit and then having someone "roll" in that set of WebKit changes.

[Luiz Agostini]

(In reply to comment #50)
> I've got bad news.  It looks like my revert fixed the problems I was seeing on the bots, which means that this patch was indeed the culprit.  Before reverting, I skimmed through (which is how I saw the one memory leak I already pointed out), but I didn't see anything else.  The fact that this was only showing up as intermittent errors on only a couple of the bots (and I couldn't even reproduce it locally) means it's probably a race or memory issue of some sort.  Is there any weak reference/pointer in your code that might be getting accessed after the memory is freed or something?

Did the problem show up in release builds or just in debug builds? I guess that your local build, that never reproduced the issue, is a release build. Could you confirm it? Could it be the cause of an apparent intermittency? 

> 
> I really wish I could point you towards more info, but unfortunately that roll is already off the console view of Chromium's waterfall, so I can only access it from work tomorrow.  I know that it won't be a whole lot of info to go on though.  And unfortunately there's no good way to try a change on the full battery of Chromium bots without committing to WebKit and then having someone "roll" in that set of WebKit changes.

[Jeremy Orlow]

(In reply to comment #51)
> (In reply to comment #50)
> > I've got bad news.  It looks like my revert fixed the problems I was seeing on the bots, which means that this patch was indeed the culprit.  Before reverting, I skimmed through (which is how I saw the one memory leak I already pointed out), but I didn't see anything else.  The fact that this was only showing up as intermittent errors on only a couple of the bots (and I couldn't even reproduce it locally) means it's probably a race or memory issue of some sort.  Is there any weak reference/pointer in your code that might be getting accessed after the memory is freed or something?
> 
> Did the problem show up in release builds or just in debug builds? I guess that your local build, that never reproduced the issue, is a release build. Could you confirm it? Could it be the cause of an apparent intermittency? 

The issues only showed up in debug builds.  Mostly with debug only assertions in a random set of tests.  1 layout test consistently failed on a mac bot.  2 layout tests failed half the time on a linux bot.  A couple other Chromium tests failed as well but had errors that didn't mean much to me at the time (I'll double check tomorrow).  I tried reproducing the mac error on my mac using a debug build but could not.  Most errors show up on our WebKit "canaries" (which run a good portion of our tests with tip of tree WebKit with tip of tree Chromium), but these did not.  It's actually pretty rare for us to see things like these in our main set of bots (build.chromium.org) and not on canaries or webkit.org bots.

I'm on #webkit right now in case you have additional questions.  I'll do what research I can tomorrow when back in the office.  But I don't think I'm going to have a ton of additional info or repro suggestions for you.  I'm also happy to do another review of the code in case I can see something via inspection.

> 
> > 
> > I really wish I could point you towards more info, but unfortunately that roll is already off the console view of Chromium's waterfall, so I can only access it from work tomorrow.  I know that it won't be a whole lot of info to go on though.  And unfortunately there's no good way to try a change on the full battery of Chromium bots without committing to WebKit and then having someone "roll" in that set of WebKit changes.

[Jeremy Orlow]

Oh, also, at first I guessed that the issue was just flake but stuff consistently failed for several iterations and then went away when I rolled out the WebKit roll.  And stayed away when I rolled it again without your changes.  If we're really stumped, we can try committing the whole thing and rolling it in again (maybe Pavel can do it at night when things are otherwise nice and quiet?), but I'm pretty sure we'll see the same issues come back.

[Luiz Agostini]

Committed r64513: <http://trac.webkit.org/changeset/64513>

[Luiz Agostini]

atwilson, pfeldman are you around?

[Simon Hausmann]

Removing this from the qtwebkit-2.1 master bug, as this isn't really needed for the release. We can reconsider if otherwise cherry-picks become really difficult.