Closed
Bug 472189
Opened 16 years ago
Closed 16 years ago
nsProgressMeterFrame causes ASSERTION: killing mutation events: 'nsContentUtils::IsSafeToRunScript()' when setting progressmetter's value
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: asqueella, Assigned: asqueella)
References
Details
(Keywords: assertion, testcase)
Attachments
(2 files, 1 obsolete file)
(deleted),
application/vnd.mozilla.xul+xml
|
Details | |
(deleted),
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•16 years ago
|
||
Assignee: nobody → asqueella
Status: NEW → ASSIGNED
Attachment #355451 -
Flags: superreview?(roc)
Attachment #355451 -
Flags: review?(enndeakin)
Comment 2•16 years ago
|
||
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
Attachment #355451 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 3•16 years ago
|
||
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.
One solution might be to add a nsReflowFrameRunnable, that might be useful in
other situations too.
Assignee | ||
Comment 5•16 years ago
|
||
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.
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?
Assignee | ||
Comment 7•16 years ago
|
||
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.
Attachment #355503 -
Flags: review?(enndeakin)
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.
Assignee | ||
Comment 9•16 years ago
|
||
Well, this particular case is about progress updates of a XUL progress meter, so hopefully it's not performance-critical.
Comment 10•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #355451 -
Flags: review?(enndeakin)
Assignee | ||
Comment 11•16 years ago
|
||
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?
Updated•16 years ago
|
Attachment #355503 -
Flags: review?(enndeakin) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #355451 -
Attachment is obsolete: true
Pushed 3afab626d3aa
Sorry, that was changeset afec2ee5b993
We proably
Flags: in-testsuite+
Roc, was that last comment cut off?
No, of course not :-)
You need to log in
before you can comment on or make changes to this bug.
Description
•