Closed Bug 376109 Opened 17 years ago Closed 17 years ago
Hang on page with five or more Flash objects
Open a page with five or more references to Flash objects (such as www.bankisrael.gov.il or the attached testcase), and the browser hangs indefinitely. (This happens almost always. If it doesn't, refreshing the page ensures a hang). This regressed between 2007-03-22 and 2007-03-23, so I'm guessing bug 345627.
> so I'm guessing bug 345627. You're right :-( I tested with a build whose source was pulled on 2006-03-08, to which I'd added my bug 345627 patch, and saw the hang (with your testcase). I have an idea for a fix. But bug 345627 is still marked security sensitive, so I'll discuss it there.
Here's a patch that works in my tests -- it doesn't hang even in an edited version of Uri's testcase that loads 20 (empty) flash plugins. (It's against the current trunk.) I expect the problem was that NSPortMessage sendBeforeDate was being called too often, too quickly. I suppose this is ultimately an Apple bug, but it's easy enough to work around. I now set mSkippedNativeCallback whenever a call to NativeEventCallback() is "skipped" (in ProcessGeckoEvents()), and only call ScheduleNativeEventCallback() (when mSuspendNativeCount reaches zero) if mSkippedNativeCallback is true. Uri, if possible please try this new patch. (I also fixed widget/src/mac, though I haven't tested this part of the fix.) (I've moved my patch and this comment from bug 345627.)
> I expect the problem was that NSPortMessage sendBeforeDate was being > called too often, too quickly. I suppose this is ultimately an > Apple bug, but it's easy enough to work around. I should add that my hang takes place in nsAppShell.mm's ScheduleNativeEventCallback(), when it's called from ResumeNative(), at the following line: [message sendBeforeDate:[NSDate distantFuture]];
Attachment #260248 - Flags: review?(joshmoz)
Comment on attachment 260248 [details] [diff] [review] Fix for bug 376109 + if (NS_SUCCEEDED(retval) && (mSuspendNativeCount == 0) && + mSkippedNativeCallback) Please align the argument on the second line with the beginning of the argument on the first line like this: if (foo && bar) ... Otherwise looks good! Thanks.
Attachment #260248 - Flags: review?(joshmoz) → review+
Here it is.
Comment on attachment 260352 [details] [diff] [review] Revised fix (format changes) >diff -U 10 -r src.old/widget/src/cocoa/nsAppShell.mm src/widget/src/cocoa/nsAppShell.mm > nsAppShell::ResumeNative(void) >+ mSkippedNativeCallback = false; PRTypes should use PRValues. Same comment applies elsewhere in the patch.
Attachment #260352 - Flags: review?(mark) → review+
What's next, folks? :-) Who should do the superreview?
Attachment #260368 - Flags: superreview?(pavlov)
Attachment #260368 - Flags: superreview?(pavlov) → superreview+
Assignee: nobody → smichaud
Whiteboard: [checkin needed]
landed on trunk
Whiteboard: [checkin needed]
Note that the widget/src/mac code here can't actually compile... (uses a non-static member in a static method).
Re-opening, because the patch doesn't compile for non-Cocoa OSX builds (as bz has just noted in comment 11).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry, dumb mistake. This should fix it.
Attachment #260368 - Attachment is obsolete: true
I landed the revised fix on the trunk, but afaik trunk carbon wouldn't compile anyway, and if it did it wouldn't run in a useful way. I could be wrong though...
What's left to do here? Are there still compile problems? If the last patch needs to land it needs a review request.
Minusing for now. If anyone has more input and feel it should block feel free to renominate.
Flags: blocking1.9? → blocking1.9-
Closing this because I don't think there are any remaining issues.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.