472189 - nsProgressMeterFrame causes ASSERTION: killing mutation events: 'nsContentUtils::IsSafeToRunScript()' when setting progressmetter's value

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Nickolay_Ponomarev]

Attachment: testcase

Load the attached XUL file in a debug build. It causes assertion "ASSERTION: killing mutation events: 'nsContentUtils::IsSafeToRunScript()'", because nsProgressMeterFrame::AttributeChanged synchronously changes other attributes:

http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsProgressMeterFrame.cpp#113

The best I can do about this is move the content updating code to run a little later. Or perhaps these attribute changes can be done with aNotify = PR_FALSE?

Comment 1

[Nickolay_Ponomarev]

Attachment: possible fix

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Could you perhaps use nsSetAttrRunnable or nsUnsetAttrRunnable with
nsContentUtils::AddScriptRunner
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.h?mark=961-984#961

Comment 3

[Nickolay_Ponomarev]

I've seen those, but here we need to set two attributes, then make this FrameNeedsReflow call... I'm not familiar with this code, so not sure about FrameNeedsReflow - if it's still needed.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

One solution might be to add a nsReflowFrameRunnable, that might be useful in
other situations too.

Comment 5

[Nickolay_Ponomarev]

I'm not sure if posting multiple runables is not a problem and if they are guaranteed to run in the post order.

If so, I can surely use nsSetAttrRunnable and add an nsReflowFrameRunnable (with params as in FrameNeedsReflow).

I'd prefer to add a common helper class (instead of the specific one in my patch) only if there are known places where it can be used.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

They are guaranteed to run in order yes. I don't know of any other places where we'd use nsReflowFrameRunnable as of yet no. But is progressmeter really that specific in what it does? Don't we have the same pattern elsewhere?

Comment 7

[Nickolay_Ponomarev]

Attachment: as suggested by smaug and sicking

The reason I was interested in other places where nsReflowFrameRunnable can be used is that it's not obvious to me that posting multiple runnables (3 in this case, possibly more in other places) is an acceptable (performance-wise) general solution compared to custom classes or to another better solution I haven't thought of.

Anyway, take your pick.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Indeed, performance might be an issue if this is something that is expected to happen a lot. I don't know the code well enough to know how often this is expected to happen.

Comment 9

[Nickolay_Ponomarev]

Well, this particular case is about progress updates of a XUL progress meter, so hopefully it's not performance-critical.

Comment 10

[Neil Deakin]

The reflow was added by dbaron in bug 253364. You might ask him why special handling is needed for progressmeters, as normally changing the flex attribute causes a reflow already.

Otherwise this seems OK I suppose.

Comment 11

[Nickolay_Ponomarev]

Neil, I checked that disabling that code breaks progressmeter. I don't think figuring out why is in scope of this bug, I can file a separate one if you want me to.

Do I have your r+ on the "as suggested by smaug and sicking" version?

Comment 12

[Nickolay_Ponomarev]

Thanks!

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 3afab626d3aa

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, that was changeset afec2ee5b993

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We proably

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Roc, was that last comment cut off?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, of course not :-)