538619 - Fennec Crashes on N900 when Lycos Mail is browsed.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[LonelyBob]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2pre) Gecko/20100108 Namoroka/3.6pre
Build Identifier: Gecko/20100106 Firefox/3.6pre Fennec/1.0

Log Into Lycos Mail. Read one of your new messages. Click on Check Mail, view returns to first login page showing new messages. Click Check Mail again, Fennec Crashes.
It appears to be a Flash related crash, when Plugins are disabled Fennec doesn't crash on Lycos Mail.
The page that is reloading seems to have only one flash file on it.
The file is located here, http://mailimg.hanmail.net/05mail/mail_bar.swf?scale=0

Reproducible: Always

Steps to Reproduce:
1. Log into Lycos Mail
2. Click on one of your new mail messages.
3. Click on link that reads "Check Mail" 
4. Click on link that reads "Check Mail" again.


Actual Results:  
Fennec crashes.

Expected Results:  
Fennec does not crash. 
New Mail messages are visible on page.

Each time the Check Mail link is pressed the New Message page is shown. The crash might be caused by Flash, and the page seems to have only one flash file on it.
The file is located here,
http://mailimg.hanmail.net/05mail/mail_bar.swf?scale=0

Comment 1

[LonelyBob]

Attachment: Test Data - Lycos Mail Saved locally.

Lycos Mail page, saved locally and zipped. Extract the Zip into a folder and open it in Fennec on N900. Click 'Check Mail' afew times and Fennec will crash.

Comment 2

[Doug Turner (:dougt)]

awesome lonelybob.

Comment 3

[Doug Turner (:dougt)]

The plugin calls InvalidateRect on a nsPluginInstanceOwner after the nsPluginInstanceOwner has been destroyed.  It looks like we messed up the reference counting on the instance owner because now the FE is holding onto the dom element associated with the plugin.

A while a back we added code to the the front end code of fennec which is notified about plugin creation, reflow, and destruction.  This code now holds a reference to the DOM Element passed in these notifications.  During the destruction event, fennec removes the DOM Element from its array, but that is sometimes too late (it isn't gc'ed away?).

The simpliest solution which requires no platform changes is to stop cacheing these elements, and use the reflow notification to recalculate the plugin cache.

I created a patch that does basically this.  It simplifes the code a big (no more cache management), and it is pretty cheap (querySelectorAll usually under 2ms).

I also noticed that during page load, we are calling updateEmbedRegions.  This actually does a draw (something that is bad at pageload time).  Is that setTimeout() the right thing?

Comment 4

[Doug Turner (:dougt)]

(maybe that pageload statement is incorrect since clip is zero)

Comment 5

[Doug Turner (:dougt)]

Attachment: patch v.1

patch w/ timing code.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 421310 [details] [diff] [review]
patch v.1

ignoring the timing code, anything else we should consider doing.  I am somewhat worried about calling querySelectorAll() during page load.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 421310 [details] [diff] [review]
patch v.1


>+        if (data == "reflow") {
>+            dump("reflow\n");
>+            this.updateCurrentBrowser();

2 spaces for indents

>   handleEvent: function handleEvent(ev) {

>+        let pluginCache = oldDoc.querySelectorAll("embed", "object");
>+        if (pluginCache)
>+          this.updateEmbedRegions(pluginCache, this._emptyRect);

Why are we doing this here? The pluginCache is made from the browser document of the tab we unselected.

Comment 8

[Benjamin Stover (:stechz)]

Attachment: Small changes

Added a check to make sure we weren't doing queryselectall when not rendering.  This will save us a lot of calls during loading.  Profiling suggests the cost is insignificant for loading time.

Also fixed a bug where we were still rendering flash if user was panning around in a div, causing flash to freak out a little on youtube.

Comment 9

[Benjamin Stover (:stechz)]

> Why are we doing this here? The pluginCache is made from the browser document
> of the tab we unselected.

We have to tell all the flash plugins on the other page to stop rendering.

(sorry for bug spam)

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 421340 [details] [diff] [review]
Small changes


> Browser.MainDragger.prototype = {

>   dragStart: function dragStart(clientX, clientY, target, scroller) {
>+    this.bv.pauseRendering();
>+
>+    // XXX shouldn't know about observer
>+    // adding pause in pauseRendering isn't so great, because tiles will hardly ever prefetch while
>+    // loading state is going (and already, the idle timer is bigger during loading so it doesn't fit
>+    // into the aggressive flag).
>+    this.bv._idleServiceObserver.pause();
>+

Remove from this patch

>     if (element)
>       this.draggedFrame = element.ownerDocument.defaultView;
>-
>-    this.bv.pauseRendering();
>-
>-    // XXX shouldn't know about observer
>-    // adding pause in pauseRendering isn't so great, because tiles will hardly ever prefetch while
>-    // loading state is going (and already, the idle timer is bigger during loading so it doesn't fit
>-    // into the aggressive flag).
>-    this.bv._idleServiceObserver.pause();

Remove from this patch

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 421340 [details] [diff] [review]
Small changes

no need for:

+        let start = Date.now();

rename "pluginCache" to thePlugins or something.  it isn't a cache any longer.

Question:

Since we are preveing updateEmbedRegions from being called during page load, do we need to call it immediately after the page finishes loading?  I am worried about ignoring plugins while the page is loading, then they never show up because we fail to call updateEmbedRegions.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

pluginCache -> plugins is good enough. "thePlugins sounds like a TV show.

Comment 13

[Benjamin Stover (:stechz)]

We are not ignoring updateEmbedRegions during page load.  Merely, we don't call updateEmbedRegions if we don't have to (most of the time during page load, rendering is paused and we don't have to).

Comment 14

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mobile-browser/rev/c5693a45cb7a