Closed Bug 472730 Opened 11 years ago Closed 11 years ago

window.sizeToContent() causes hang & full CPU usage, due to extreme recursion

Categories

(Core :: General, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1
Tracking Status
status1.9.1 --- unaffected

People

(Reporter: dholbert, Assigned: smaug)

References

()

Details

(Keywords: hang, regression)

Attachments

(4 files, 3 obsolete files)

The testcase from bug 455573 (attachment 338920 [details]) causes massive
hangs using today's nightly:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090108 Minefield/3.2a1pre

This apparently regressed during the time since September (when bug 455573 was filed).

I'm initially marking this as security-sensitive, since bug 455573 is still marked as such, and I wouldn't want to give anything away about that bug while it's still marked as security-sensitive.

I'm attaching the backtrace from very soon after the recursion has started, when it's 264 levels deep.

This also (within a minute or two) starts printing error messages of the form:
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "too much recursion"]' when calling method: [nsITimerCallback::notify]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

I get a bunch of these error messages, for various methods -- e.g. [nsITimerCallback::notify], [nsIObserver::observe], [nsIWebProgressListener::onStateChange]
Argh, probably a regression from Bug 457862.
Blocks: 457862
Here's a deeper backtrace, from after I've let the testcase hang uninterrupted from a few seconds.

I had to gzip it, because it's too big for bugzilla otherwise.
(In reply to comment #1)
> Argh, probably a regression from Bug 457862.

Yup, regression range suggests that it is. Hang starts between these two builds:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090105 Minefield/3.2a1pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090106 Minefield/3.2a1pre
I'm un-hiding this bug, since it was only hidden because bug 455573 was hidden, and that bug's now public per bug 455573 comment #10.
Group: core-security
Comment on attachment 356054 [details] [diff] [review]
Fire resize asynchronously, but before flushing/painting (if safe to run scripts)

David, Boris, what you think about this?
Other option could be to have the if (mResizeEvent.IsPending()) FireResizeEvent(); in PresShell::WillPaint(), just before flushing.
This seems to make sense (though no need to null-check the event after new, imo.
Attached patch without null check (obsolete) — Splinter Review
Assignee: nobody → Olli.Pettay
Attachment #356054 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #356149 - Flags: superreview?(dbaron)
Attachment #356149 - Flags: review?(dbaron)
Attached patch s/Forget/Revoke/Splinter Review
Attachment #356149 - Attachment is obsolete: true
Attachment #356151 - Flags: superreview?(dbaron)
Attachment #356151 - Flags: review?(dbaron)
Attachment #356149 - Flags: superreview?(dbaron)
Attachment #356149 - Flags: review?(dbaron)
Attachment #356151 - Flags: superreview?(dbaron)
Attachment #356151 - Flags: superreview+
Attachment #356151 - Flags: review?(dbaron)
Attachment #356151 - Flags: review+
Comment on attachment 356151 [details] [diff] [review]
s/Forget/Revoke/

Might it be better to call Revoke in FlushPendingNotifications (before calling FireResizeEvent) and Forget in FireResizeEvent?

r+sr=dbaron either way
Attached patch Revoke also when destroying (obsolete) — Splinter Review
I commit this once the tree is open again.
Attached patch er, this oneSplinter Review
Attachment #356422 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/40ba2eac8f4b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Attachment #356797 - Flags: approval1.9.1+
Keywords: checkin-needed
Whiteboard: [needs 191 landing]
Comment on attachment 356797 [details] [diff] [review]
er, this one

Does not apply cleanly to 1.9.1:
{
patching file layout/base/nsPresShell.cpp
Hunk #1 FAILED at 1182
Hunk #2 FAILED at 1649
Hunk #3 FAILED at 2519
3 out of 4 hunks FAILED
}
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.2a1
Whiteboard: [needs 191 landing] → [needs 191 landing after bug 457862]
Just to summarize the situation here, for 1.9.1 landing: 
 - The whiteboard here says that this "needs 191 landing after bug 457862"
 - bug 457862 comment 30 & 31 says *it* won't land on 191 until after bug 473805
 - bug 473805 doesn't yet have a patch. (One was posted in Jan, but it got r-)
Flags: wanted1.9.1.x?
Comment on attachment 356797 [details] [diff] [review]
er, this one

Revoking approval. We're cutting back on potential churn here. We can try again for 3.5.1 if the dependencies iron themselves out.
Attachment #356797 - Flags: approval1.9.1+ → approval1.9.1-
Depends on: 498340
blocking1.9.1: --- → ?
status1.9.1: --- → ?
Flags: wanted1.9.1.x?
Bug 473805 now has a patch. Time to revisit this?
blocking1.9.1: ? → ---
Whiteboard: [needs 191 landing after bug 457862]
You need to log in before you can comment on or make changes to this bug.