Closed Bug 538619 Opened 10 years ago Closed 10 years ago

Fennec Crashes on N900 when Lycos Mail is browsed.

Categories

(Firefox for Android Graveyard :: General, defect, critical)

ARM
Maemo
defect
Not set
critical

Tracking

(fennec1.0+)

RESOLVED FIXED
Tracking Status
fennec 1.0+ ---

People

(Reporter: LonelyBob, Assigned: dougt)

References

()

Details

(Whiteboard: [MobFxRCTestday])

Attachments

(3 files)

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
Whiteboard: [MobFxRCTestday]
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.
awesome lonelybob.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
(maybe that pageload statement is incorrect since clip is zero)
tracking-fennec: --- → 1.0+
Attached patch patch v.1Splinter Review
patch w/ timing code.
Assignee: nobody → mozbugz
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.
Attachment #421310 - Flags: review?(webapps)
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.
Attached patch Small changesSplinter Review
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.
Attachment #421340 - Flags: review?(mark.finkle)
Attachment #421310 - Flags: review?(webapps)
Attachment #421340 - Flags: review?(mozbugz)
> 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 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
Attachment #421340 - Flags: review?(mark.finkle) → review+
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.
Attachment #421340 - Flags: review?(mozbugz) → review+
pluginCache -> plugins is good enough. "thePlugins sounds like a TV show.
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).
Pushed http://hg.mozilla.org/mobile-browser/rev/c5693a45cb7a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Component: Linux/Maemo → General
OS: Other → Linux (embedded)
QA Contact: maemo-linux → general
Hardware: Other → ARM
You need to log in before you can comment on or make changes to this bug.