Closed Bug 376109 Opened 16 years ago Closed 16 years ago

Hang on page with five or more Flash objects

Categories

(Core Graveyard :: Plug-ins, defect)

PowerPC
macOS
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: uriber, Assigned: smichaud)

References

()

Details

(Keywords: hang, regression, testcase)

Attachments

(2 files, 3 obsolete files)

Attached file testcase
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.
Flags: blocking1.9?
> 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.
Attached patch Fix for bug 376109 (obsolete) — Splinter Review
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+
Attachment #260248 - Flags: review?(mark)
Attached patch Revised fix (format changes) (obsolete) — Splinter Review
Here it is.
Attachment #260248 - Attachment is obsolete: true
Attachment #260352 - Flags: review?(mark)
Attachment #260248 - Flags: review?(mark)
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+
Attached patch Revised fix (use PRValues) (obsolete) — Splinter Review
Attachment #260352 - Attachment is obsolete: true
What's next, folks? :-)

Who should do the superreview?
Attachment #260368 - Flags: superreview?(pavlov)
Duplicate of this bug: 376330
Attachment #260368 - Flags: superreview?(pavlov) → superreview+
Assignee: nobody → smichaud
Whiteboard: [checkin needed]
landed on trunk
Whiteboard: [checkin needed]
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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: 16 years ago16 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.