376109 - Hang on page with five or more Flash objects

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Uri Bernstein (Google)]

Attachment: 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.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

> 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.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix for bug 376109

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.)

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> 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]];

Comment 4

[Josh Aas]

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.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Attachment: Revised fix (format changes)

Here it is.

Comment 6

[Mark Mentovai]

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.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Attachment: Revised fix (use PRValues)

Comment 8

[Steven Michaud [:smichaud] (Retired)]

What's next, folks? :-)

Who should do the superreview?

Comment 10

[Josh Aas]

landed on trunk

Comment 11

[Boris Zbarsky [:bzbarsky]]

Note that the widget/src/mac code here can't actually compile... (uses a non-static member in a static method).

Comment 12

[Andrew Miller]

Re-opening, because the patch doesn't compile for non-Cocoa OSX builds (as bz has just noted in comment 11).

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Attachment: Revised fix (use "self->")

Sorry, dumb mistake.  This should fix it.

Comment 14

[Josh Aas]

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...

Comment 15

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

What's left to do here? Are there still compile problems? If the last patch needs to land it needs a review request.

Comment 16

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

Minusing for now. If anyone has more input and feel it should block feel free to renominate.

Comment 17

[Josh Aas]

Closing this because I don't think there are any remaining issues.