Re: [Interest] invalidated copies of QString in QCommonStyle::cachedOption

[Thiago Macieira]

On Wednesday 20 January 2016 12:34:33 Mojmír Svoboda wrote:
> On 20.1.2016 11:26, Igor Mironchik wrote:
> > Just looked a little at your code. And that what I can say: your code
> > has nothing with QStringLiteral.
> 
> QStringDataTable uses the same technique as QStringLiteral.

There's no such class. Don't name your classes with Q + capital letter, 
please.

> > And I don't understand why you need such a "crazy" :) code? May be would
> > be much more better and effective to implement custom model and just
> > store in the model normal QStrings?
> 
> Normally these strings are read-only so there is no reason to allocate
> them on a heap. And there are lots of them (may exceed order 10^6)
> Then I met the article I mentioned earlier so I went for it, because it
> seems like a neat feature... what could possibly go wrong? :)

Just don't unmap your file. Ever.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center

___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest

Re: [Interest] invalidated copies of QString in QCommonStyle::cachedOption

[Mojmír Svoboda]

On 20.1.2016 11:26, Igor Mironchik wrote:


Just looked a little at your code. And that what I can say: your code
has nothing with QStringLiteral.


QStringDataTable uses the same technique as QStringLiteral.


And I don't understand why you need such a "crazy" :) code? May be would
be much more better and effective to implement custom model and just
store in the model normal QStrings?


Normally these strings are read-only so there is no reason to allocate 
them on a heap. And there are lots of them (may exceed order 10^6)
Then I met the article I mentioned earlier so I went for it, because it 
seems like a neat feature... what could possibly go wrong? :)



And you forgot to delete selection model! Seems that this cache of
QString exists because selection model use it?!


oh yeah, i indeed forgot. will try.

cheers,
Mojmir
___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest

Re: [Interest] invalidated copies of QString in QCommonStyle::cachedOption

[Igor Mironchik]

Hi,

On 20.01.2016 12:45, Mojmír Svoboda wrote:

On 19.1.2016 10:37, Giuseppe D'Angelo wrote:


Please trim down to a small testcase and submit a bug report. If your
model is well-behaving the view can't cache the option that way.


I managed to reproduce the behaviour with QStandardItemModel, so here
is the repro case:
https://bugreports.qt.io/browse/QTBUG-50551


Just looked a little at your code. And that what I can say: your code 
has nothing with QStringLiteral.


And I don't understand why you need such a "crazy" :) code? May be would 
be much more better and effective to implement custom model and just 
store in the model normal QStrings?


And you forgot to delete selection model! Seems that this cache of 
QString exists because selection model use it?!




Many thanks guys,
Mojmir
___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest


___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest

Re: [Interest] invalidated copies of QString in QCommonStyle::cachedOption

[Mojmír Svoboda]

On 20.1.2016 11:38, Igor Mironchik wrote:


Why are you deleting model?


i am trying to remove everything i can so that there are no references
to the underlying data (m_strings).


Just implement clear() method and between
beginResetModel(), endResetModel() delete your data. And I guess that
this crash will not occur again.


not sure if it will help, can try. but the value is cached in 
QCommonStyle, not the model.


Mojmir
___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest

Re: [Interest] invalidated copies of QString in QCommonStyle::cachedOption

[Igor Mironchik]

On 20.01.2016 14:25, Mojmír Svoboda wrote:

On 20.1.2016 11:38, Igor Mironchik wrote:


Why are you deleting model?


i am trying to remove everything i can so that there are no references
to the underlying data (m_strings).


Just implement clear() method and between
beginResetModel(), endResetModel() delete your data. And I guess that
this crash will not occur again.


not sure if it will help, can try. but the value is cached in 
QCommonStyle, not the model.


When you call beginResetModel() and endResetModel() view clears 
everything cached...




Mojmir
___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest


___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest

Re: [Interest] invalidated copies of QString in QCommonStyle::cachedOption

[Mojmír Svoboda]

On 19.1.2016 10:37, Giuseppe D'Angelo wrote:


Please trim down to a small testcase and submit a bug report. If your
model is well-behaving the view can't cache the option that way.


I managed to reproduce the behaviour with QStandardItemModel, so here
is the repro case:
https://bugreports.qt.io/browse/QTBUG-50551

Many thanks guys,
Mojmir
___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest

Re: [Interest] invalidated copies of QString in QCommonStyle::cachedOption

[Igor Mironchik]

On 20.01.2016 12:45, Mojmír Svoboda wrote:

On 19.1.2016 10:37, Giuseppe D'Angelo wrote:


Please trim down to a small testcase and submit a bug report. If your
model is well-behaving the view can't cache the option that way.


I managed to reproduce the behaviour with QStandardItemModel, so here
is the repro case:
https://bugreports.qt.io/browse/QTBUG-50551


Why are you deleting model? Just implement clear() method and between 
beginResetModel(), endResetModel() delete your data. And I guess that 
this crash will not occur again.




Many thanks guys,
Mojmir
___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest


___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest

Re: [Interest] invalidated copies of QString in QCommonStyle::cachedOption

[Igor Mironchik]

On 20.01.2016 14:34, Mojmír Svoboda wrote:

On 20.1.2016 11:26, Igor Mironchik wrote:


Just looked a little at your code. And that what I can say: your code
has nothing with QStringLiteral.


QStringDataTable uses the same technique as QStringLiteral.


QStringLiteral uses static structs! You don't.




And I don't understand why you need such a "crazy" :) code? May be would
be much more better and effective to implement custom model and just
store in the model normal QStrings?


Normally these strings are read-only so there is no reason to allocate 
them on a heap. And there are lots of them (may exceed order 10^6)
Then I met the article I mentioned earlier so I went for it, because 
it seems like a neat feature... what could possibly go wrong? :)


I do not know. I need benchmarks for it. May be you will win nothing...




And you forgot to delete selection model! Seems that this cache of
QString exists because selection model use it?!


oh yeah, i indeed forgot. will try.

cheers,
Mojmir
___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest


___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest

Re: [Interest] invalidated copies of QString in QCommonStyle::cachedOption

[Mojmír Svoboda]

On 18.1.2016 14:48, Igor Mironchik wrote:


What if to change QStringLiteral with QLatin1String usage?


Tried it yesterday, and it works without problems as the
reference count in QStringData ensures lifetime of the QString.

I also tried some very dirty way to release the cachedOption
before closing the widget and it worked on a sample. Worth
a try, but not a solution :)

Nice day,
Mojmir
___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest

Re: [Interest] invalidated copies of QString in QCommonStyle::cachedOption

[Giuseppe D'Angelo]

On Mon, Jan 18, 2016 at 9:40 AM, Mojmír Svoboda  wrote:

> One of the copies is in QCommonStylePrivate::cachedOption::text field. I
> suspect these style classes use some form of sharing, so when i close the
> widget, the copy of qstring will survive the storage, and then when new
> qtableview opens, it tries to reuse the style and on the first use it
> destroys the old content of .text field and crash follows.
>

That is correct. It's an unfortunate side effect of QStrings built like
QStringLiteral does. We've seen that already when unloading plugins which
have handed out QStrings...


> Now what do i do? Try to force somehow the style class to drop the
> d->cachedOption?
> Any ideas, please?
>

Please trim down to a small testcase and submit a bug report. If your model
is well-behaving the view can't cache the option that way. You would have
crashes even if you use entirely public APIs (such as QString::fromRawData)
and that's unacceptable.

HTH,
-- 
Giuseppe D'Angelo
___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest

Re: [Interest] invalidated copies of QString in QCommonStyle::cachedOption

[Igor Mironchik]

On 18.01.2016 13:46, Mojmír Svoboda wrote:

On 18.1.2016 10:21, Igor Mironchik wrote:


Can you reset model on closing of your table view, drop any data, etc...
And then close view.


Yes i am doing that. I am trying to close everything i know
i created (models, proxy models, context menus, delegates).
But the problem persists.


What if to change QStringLiteral with QLatin1String usage?



While inpecting the code i came to think that this class 
(QWindowsVistaStyle in current case) is not deleted when i close my 
table, but rather shared between multiple widgets. If i take

QStyle * s0 =  m_main_window->dockManager().style(); for example
where dockManager() is an unrelated QTreeView, the .text field in this
s0 contains .text from completely different QTableView (the one that
i am closing).


If I understood your correctly you are loading model again on new show?


i have one widget right from start, this is the dockManager with the 
style s0. This widget is the first to create and therefore
i think it's the first to create the style via QStyleFactory. Some 
(all?) subsequent views seems to re-use this s0.


And then there is this QTableView that opens when tcp connection with 
data comes and closed on new connection with new data or user request.


Mojmir
___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest


___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest

[Interest] invalidated copies of QString in QCommonStyle::cachedOption

[Mojmír Svoboda]

Hello,

i have a crash in QCommonStyle::subElementRect

case SE_ItemViewItemDecoration:
case SE_ItemViewItemText:
case SE_ItemViewItemFocusRect:
if (const QStyleOptionViewItem *vopt = ...) {
if (!d->isViewItemCached(*vopt)) {
d->viewItemLayout(...);
if (d->cachedOption) {
delete d->cachedOption;
d->cachedOption = 0;
}
d->cachedOption = new QStyleOptionViewItem(*vopt);
}

with callstack:
  QArrayData::deallocate
  QTypedArrayData::deallocate(QArrayData * data)
  QString::~QString() Line 1070
  QStyleOptionViewItem::~QStyleOptionViewItem()
  QStyleOptionViewItem::`scalar deleting destructor'(unsigned int)
> QCommonStyle::subElementRect(QStyle::SubElement sr, const 
QStyleOption * opt, const QWidget * widget) Line 2993

  QWindowsStyle::subElementRect
  QWindowsVistaStyle::subElementRect
  QCommonStyle::drawControl
  QWindowsStyle::drawControl
  QWindowsVistaStyle::drawControl
  QStyledItemDelegate::paint
  QTableViewPrivate::drawCell
  QTableView::paintEvent
  ...


How this occurs:
i have QTableView with custom model that uses memmapped file filled with 
lots of QStringData. These QStringData are set using the technique 
presented in http://woboq.com/blog/qstringliteral.html for example.


The problem is when the storage goes away (widget is closed), because 
there may be copies of those strings, and indeed they are.


One of the copies is in QCommonStylePrivate::cachedOption::text field. I
suspect these style classes use some form of sharing, so when i close 
the widget, the copy of qstring will survive the storage, and then when 
new qtableview opens, it tries to reuse the style and on the first use 
it destroys the old content of .text field and crash follows.


Now what do i do? Try to force somehow the style class to drop the 
d->cachedOption?

Any ideas, please?

Best regards,
Mojmir
___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest

Re: [Interest] invalidated copies of QString in QCommonStyle::cachedOption

[Mojmír Svoboda]

On 18.1.2016 10:21, Igor Mironchik wrote:


Can you reset model on closing of your table view, drop any data, etc...
And then close view.


Yes i am doing that. I am trying to close everything i know
i created (models, proxy models, context menus, delegates).
But the problem persists.

While inpecting the code i came to think that this class 
(QWindowsVistaStyle in current case) is not deleted when i close my 
table, but rather shared between multiple widgets. If i take

QStyle * s0 =  m_main_window->dockManager().style(); for example
where dockManager() is an unrelated QTreeView, the .text field in this
s0 contains .text from completely different QTableView (the one that
i am closing).


If I understood your correctly you are loading model again on new show?


i have one widget right from start, this is the dockManager with the 
style s0. This widget is the first to create and therefore
i think it's the first to create the style via QStyleFactory. Some 
(all?) subsequent views seems to re-use this s0.


And then there is this QTableView that opens when tcp connection with 
data comes and closed on new connection with new data or user request.


Mojmir
___
Interest mailing list
Interest@qt-project.org
http://lists.qt-project.org/mailman/listinfo/interest