Last Comment Bug 466586 - eBay preview image occasionally disappears after briefly appearing
: eBay preview image occasionally disappears after briefly appearing
Status: VERIFIED FIXED
: regression, verified1.9.1
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: P1 major (vote)
: mozilla1.9.1b4
Assigned To: Joe Drew (not getting mail)
:
Mentors:
http://cgi.ebay.com/ebaymotors/Cars-T...
: 301279 324405 (view as bug list)
Depends on: 476349 476457 479328 479502 480352
Blocks: 490384
  Show dependency treegraph
 
Reported: 2008-11-24 18:12 PST by Stephen Donner [:stephend]
Modified: 2013-03-25 15:23 PDT (History)
22 users (show)
roc: blocking1.9.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (saved HTML) (129.07 KB, text/html)
2008-11-24 21:32 PST, Daniel Holbert [:dholbert]
no flags Details
testcase that alerts when fails (129.20 KB, text/html)
2008-12-08 17:10 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
352016: testcase that alerts when fails, part 2 (39.84 KB, text/html)
2008-12-10 09:40 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
When possible, resurrect cache entries that have been evicted (18.76 KB, patch)
2009-01-13 15:49 PST, Joe Drew (not getting mail)
no flags Details | Diff | Splinter Review
Only track image cache entries whose requests have no observers (23.92 KB, patch)
2009-01-15 14:50 PST, Joe Drew (not getting mail)
vladimir: superreview+
Details | Diff | Splinter Review
Address review comments (25.76 KB, patch)
2009-01-20 15:55 PST, Joe Drew (not getting mail)
pavlov: review+
vladimir: superreview+
Details | Diff | Splinter Review
Test to ensure we don't need to reload an image in a document (8.07 KB, patch)
2009-01-22 15:55 PST, Joe Drew (not getting mail)
no flags Details | Diff | Splinter Review
Fix leaks in previous patch (29.39 KB, patch)
2009-01-23 15:23 PST, Joe Drew (not getting mail)
joe: review+
vladimir: superreview+
Details | Diff | Splinter Review
Fix obscure leak by checking for eviction (31.11 KB, patch)
2009-01-30 16:04 PST, Joe Drew (not getting mail)
joe: review+
vladimir: superreview+
Details | Diff | Splinter Review
Check the return value of nsExpirationTracker::AddObject (31.50 KB, patch)
2009-02-10 19:03 PST, Joe Drew (not getting mail)
vladimir: review+
Details | Diff | Splinter Review
Related crash stack? (1.43 KB, patch)
2009-02-20 02:10 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review

Description Stephen Donner [:stephend] 2008-11-24 18:12:49 PST
Summary: eBay preview image occasionally disappears after briefly appearing

URL: http://cgi.ebay.com/ebaymotors/Cars-Trucks___1-OWNER-NAVIGATION-REAR-BACK-UP-CAMERA-A-C-SEATS_W0QQitemZ110315606502QQddnZCarsQ20Q26Q20TrucksQQddiZ2282QQcmdZViewItemQQptZUS_Cars_Trucks?hash=item110315606502&_trksid=p4506.c0.m245&_trkparms=72%3A727|65%3A12|39%3A1|240%3A1308

Steps to Reproduce:

1. Load http://cgi.ebay.com/ebaymotors/Cars-Trucks___1-OWNER-NAVIGATION-REAR-BACK-UP-CAMERA-A-C-SEATS_W0QQitemZ110315606502QQddnZCarsQ20Q26Q20TrucksQQddiZ2282QQcmdZViewItemQQptZUS_Cars_Trucks?hash=item110315606502&_trksid=p4506.c0.m245&_trkparms=72%3A727|65%3A12|39%3A1|240%3A1308
2. If the preview image on the left stays, then force a refresh/reload using SHIFT+Reload

Actual Results:

Sometimes, the preview image disappears after it briefly appears

Expected Results:

The preview image should always remain
Comment 1 Daniel Holbert [:dholbert] 2008-11-24 21:32:27 PST
Created attachment 349922 [details]
testcase (saved HTML)

I can reproduce this at the URL within a minute or two of reloading, using
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081124 Minefield/3.1b2pre

Here's the saved HTML (which still uses a bunch of remote resources) -- I can reproduce it using this file, too.

FWIW, I tried using Firefox 2.0.0.17 and couldn't reproduce this bug within a few minutes.  Regression?
Comment 2 Stephen Donner [:stephend] 2008-11-24 21:33:51 PST
Yeah, this bug doesn't reproduce with 3.0.4, either, so it's just a Gecko 1.9.1 regression.
Comment 3 Sander 2008-11-29 14:03:30 PST
I tried making a minimal testcase here, but the more I remove from the page, the harder it gets to reproduce. It seems to be either a race issue, or perhaps related to the cache filling up?

The relevant javascript is:
function standardizeImage(image,dimension){
var customImage=new Image();customImage.src=image.src;var imw;var imh;
imw=customImage.width;imh=customImage.height;
var rw=imw/dimension,rh=imh/dimension,ratio=(rw>rh)?rw:rh;
if(ratio>=1){image.width=imw/ratio;image.height=imh/ratio;}
else{image.width=imw;image.height=imh;}}

That's called from the image: onload="standardizeImage(this,200)"
and when the image disappears, imw and imh are 0. So, somehow, sometimes right after setting customImage.src=image.src, that customImage doesn't have a width and height yet (but most of the time it does). 
(If you set the width and height of the image element to non-zero values with the dom inspector, the image shows up again.)

I suspect Safari would run into the same problem, as there's an entire branch in that function for Safari, which works on the original image rather than creating a new temporary image.
if(is.safari&&is.mac){
 var origImg=getOriginalImg();
 if(typeof(origImg)!=u) {
  if(typeof(origImg.width)!=u){imw=origImg.width;}
  if(typeof(origImg.height)!=u){imh=origImg.height;}
 }
}
(u is "undefined" there.)
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-12-08 15:53:09 PST
Could somebody find a regression range for this?  (In other words, the last nightly build before this started happening and the first one in which it started.)
Comment 5 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-12-08 17:10:53 PST
Created attachment 352016 [details]
testcase that alerts when fails

This is the same as the previously attached testcase, but one that gives an alert when the bug occurs. And it does automatic reloading. It allowed me to find the regression range easier.

I get a regression range between 2008-09-04 and 2008-09-05 (I let the 2008-09-04 build run for 10 minutes or so before I got tired of it):
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-09-04+04%3A00%3A00&enddate=2008-09-05+04%3A00%3A00
So this seems to me a regression from bug 430061 somehow.
Comment 6 Sander 2008-12-08 17:33:16 PST
I've been able to make the image intermittently not show as far back as 2007091216 (I've been testing by loading the attached testcase and the original eBay url in 30+ tabs, and giving a reload all once or twice, after which there always was at least one tab that had the image not show up).
Comment 7 Joe Drew (not getting mail) 2008-12-08 17:43:59 PST
Given comment #6, I suspect that this lies entirely in Content-land. (It's the only thing that makes sense to me, anyways.)

Content folk: feel free to punt back to me if you've got a reason to suspect it's not you.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-12-08 17:49:56 PST
So on Linux, x86_64, using attachment 352016 [details], I'm also seeing the regression as between 2008-09-04-05-mozilla-central and 2008-09-05-05-mozilla-central.
Comment 9 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-12-10 09:40:16 PST
Created attachment 352339 [details]
352016: testcase that alerts when fails, part 2

This is a bit reduced, but indeed, it seems increasingly difficult to reduce it, the smaller the file gets. I can also reproduce this in offline mode, btw.
Comment 10 Joe Drew (not getting mail) 2008-12-10 14:11:44 PST
After further discussion with Sander and Martijn, this is probably Imagelib after all.
Comment 11 Stephen Donner [:stephend] 2008-12-23 19:10:54 PST
Hrm; I think bug 468160 might have fixed this.  Martijn/Sander: do you guys still see it?
Comment 12 Stephen Donner [:stephend] 2008-12-23 19:14:44 PST
(In reply to comment #11)
> Hrm; I think bug 468160 might have fixed this.  Martijn/Sander: do you guys
> still see it?

nvm me; still happens, just really sporadically, as Martijn mentions in comment 9.
Comment 13 Joe Drew (not getting mail) 2009-01-07 14:06:02 PST
eBay's code is sort of reasonable, but I believe it's wrong.

What happens is that, with the volume of images on that page, once they're all loaded, the cache has been more than filled. The thumbnail that gets resized is one of the images that gets thrown out of the cache, so when they do customimage.src = image.src, it needs to reload from the network (or, at least, the disk cache). This means that the image isn't available until its onload handler gets called, leading to the image disappearing.

I'm inclined to resolve this invalid, but I'm willing to hear alternate points of view on it.
Comment 14 Vladimir Vukicevic [:vlad] [:vladv] 2009-01-07 14:10:11 PST
That sounds reasonable, although a little odd given that the image is actually available to be displayed.  A more complex solution would be to recognize that we have the uncompressed bits sitting around already... though I guess that won't work, the image that the server serves up could be completely different for the same URL.
Comment 15 Martijn Wargers [:mwargers] (not working for Mozilla) 2009-01-07 14:10:51 PST
We have a situation here where Firefox is acting differently after once every
xxx reloads.
Firefox should always act one way or another, not do something different once
in while. 
Or am I wrong?
Comment 16 Vladimir Vukicevic [:vlad] [:vladv] 2009-01-07 14:34:43 PST
Firefox should do whatever the script/html tells it to do -- in this case, the script is doing something erroneous.  It worked before due to an implementation quirk, but there's nothing that says that the code as written should replace the image seamlessly.  One element is removed, another one is put in its place before it's loaded...
Comment 17 Joe Drew (not getting mail) 2009-01-07 14:49:15 PST
The reason it happens occasionally rather than all the time is because of the way image caching works. We don't evict elements from the cache until another image is loaded. Most of the images from this page are removed from the cache when this happens, so it's entirely a factor of timing from the network and whatever other images are loading at the time. This is why having multiple copies of the testcase open in multiple tabs: we get different images loading at different times, so it's more likely that the image in question gets evicted before the js gets run.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-01-07 14:55:26 PST
It would work if we refused to evict any image from the cache that's currently in use, right?
Comment 19 Joe Drew (not getting mail) 2009-01-08 11:39:10 PST
Yes, but I suspect that's not a good idea: think of having a pile of flickr images open in tabs, for example. Your cache would be all full of stuff that you're probably not going to re-request (content still has a ref to it), and you wouldn't be able to add any new things to the cache.

I'm going to do an experiment by adding a second hash table of weak refs to "all cache entries currently alive." If we don't have an element in the cache, but it still exists in memory, maybe we can just throw it back into the cache.
Comment 20 Joe Drew (not getting mail) 2009-01-13 15:49:19 PST
Created attachment 356847 [details] [diff] [review]
When possible, resurrect cache entries that have been evicted

This should have been a lot easier to fix, but some stupid thinkos made it a several-day affair.

While I still maintain that this bug is not necessarily a bug, it'd be nice to be able to grab images that are still in use and use them as-is, rather than going to the network or disk. So this patch does that. I ran the ebay test with a build with this patch, and never once got an alert.

Still undetermined is whether i should aim this for 1.9.1. I'm sort of inclined not to, because it seems sort of risky in a refcount sort of way, but I'm willing to listen to arguments the other way.
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-01-13 19:44:23 PST
I'm not the right reviewer here.
Comment 22 Boris Zbarsky [:bz] 2009-01-14 07:10:20 PST
So...  It sounds like this site is using the standard image preloading setup where you do new Image() and then use that src on the page, right?  Page authors expect such images to not hit the network, and that's what we used to do as I recall.  We need to keep that behavior; breaking it is not acceptable.  So this bug is most definitely a bug.  I thought I'd said all this in an imagelib bug recently, but I can't find it.

Perhaps we shouldn't pin images in the cache from all content nodes, but only from ones that are not in a document (or even only ones created via new Image() and not in a document)?
Comment 23 Boris Zbarsky [:bz] 2009-01-14 07:17:10 PST
Er, reading comment 3 more carefully it seems that in fact they're assigning from an image in the document to a new Image().  So while the new Image() thing is still broken, and still needs to be fixed, that won't fix this bug.

That said, I'm not sure I'm following comment 19.  I thought the image cache basically cached imgRequests, right?  Would it make sense to not count imgRequests with outstanding imgRequestProxies against the cache size (so that we're only really "caching" the things that no one is actively using; we're just using the cache as a lookup table to get at the rest)?
Comment 24 Joe Drew (not getting mail) 2009-01-15 14:50:05 PST
Created attachment 357239 [details] [diff] [review]
Only track image cache entries whose requests have no observers

Boris' idea was pretty brilliant, and I couldn't help but think it's a better long-term solution for images. So I implemented it.

We now have two sets of cache entries: those that are tracked for time-based expiry and counted towards the cache limit, and the rest, which will be kept in memory anyways because they have observers holding strong references to them. 

This way, we can always serve requests for images that are currently loaded, regardless of size, and the cache becomes a cache of only currently-unused images.
Comment 25 Vladimir Vukicevic [:vlad] [:vladv] 2009-01-15 15:03:16 PST
Comment on attachment 357239 [details] [diff] [review]
Only track image cache entries whose requests have no observers

Patch looks good to me overall, just not super happy with the 'Tracked' term -- discussed some ideas on irc.
Comment 26 Vladimir Vukicevic [:vlad] [:vladv] 2009-01-15 15:12:52 PST
For joe:

14:58 < vlad> the patch looks good, but only thing is that the 'Tracked' 
              terminology was confusing to me
14:58 < joe> vlad: hm, yes
14:58 < vlad> I got to the comment which says that things that are tracked are 
              tracked for eviction
14:58 < joe> got a better name?
14:58 < vlad> so maybe call it CanBeEvicted() or IsEvictable() or something?
14:58 < vlad> IsBadTenant()? :)
14:58 < joe> ah yes, and then your brain leaked out your ears
14:59 < joe> well any entry can be removed from cache/evicted
14:59 < joe> if it's cancelled or whatever
14:59 < vlad> so if something is not tracked
14:59 < vlad> it can still be evicted?
14:59 < joe> but only by explicit request
14:59 < joe> it won't be evicted by the cache
14:59 < vlad> ok, so it won't be evicted by the normal cache eviction
14:59 < joe> for example if time passes or by space
14:59 < joe> right
15:00 < vlad> so maybe CanBeAutoEvicted or something?
15:00 < joe> hm
15:00 < joe> not superhappy with that name, but i'll mediate on it
15:00 < vlad> or even make it explicit what it is
15:00 < joe> meditate
15:00 < vlad> like HasNoObservers()
15:00 < joe> hmm
15:00 < joe> that's interesting
15:01 < vlad> because what you really want to be able to say is "Only things 
              that have no observers will be automatically evicted by the 
              cache; other entries can only be explicitly evicted due to 
              request cancellation or other events"
15:01 < vlad> or something like that
15:02 < vlad> then AddTrackedItem() becomes SetHasNoObservers() etc.
15:08 < joe> can you put all this onto the bug?
15:10 < vlad> I referenced the irc conversation, want me to copy & paste?
15:12 < joe> sure
Comment 27 Boris Zbarsky [:bz] 2009-01-15 16:39:22 PST
Comment on attachment 357239 [details] [diff] [review]
Only track image cache entries whose requests have no observers

>The image cache is now comprised of two things: a heap of tracked items that

s/comprised/composed/.  Or "now comprises two things".

>+++ b/modules/libpr0n/src/imgLoader.cpp
>+PRBool imgLoader::AddTrackedItem(nsIURI *key, imgCacheEntry *entry)
...
>+  entry->SetTracked(true);

PR_TRUE.

>+PRBool imgLoader::RemoveTrackedItem(nsIURI *key)
...
>+    entry->SetTracked(false);

PR_FALSE.

>@@ -1033,24 +1116,33 @@ NS_IMETHODIMP imgLoader::LoadImage(nsIUR
>+        // If this entry is being tracked, it's an entry for a request that
>+        // has no proxies currently, and the request has no reference to the entry.
>+        if (entry->Tracked()) {

...

>@@ -1168,43 +1260,54 @@ NS_IMETHODIMP imgLoader::LoadImageWithCh
>         request = getter_AddRefs(entry->GetRequest());
>+
>       } else {

Nix the random whitespace change.

>+        // If this entry isn't being tracked, it's an entry for a request that
>+        // has no proxies currently, and the request has no reference to the entry.
>+        if (entry->Tracked()) {

Why the comment difference from LoadImage()?  I'd think the comments should say the same things...

In terms of what to call this, I like HasNoObservers() or maybe IsInUse() (with the opposite meaning, of course).
Comment 28 Joe Drew (not getting mail) 2009-01-20 15:55:01 PST
Created attachment 357879 [details] [diff] [review]
Address review comments

This code is going to need significant baking on the trunk, I think; it's currently a bit fragile, in that it's quite easy to add and remove crasher bugs. Most of those problems have lain in imgCacheValidator, so please focus on that in review.
Comment 29 Vladimir Vukicevic [:vlad] [:vladv] 2009-01-21 14:24:33 PST
Comment on attachment 357879 [details] [diff] [review]
Address review comments

Nothing really bad jumps out at me, but I don't have my head fully wrapped around the interaction between the different objects in imglib.
Comment 30 Joe Drew (not getting mail) 2009-01-22 15:55:08 PST
Created attachment 358292 [details] [diff] [review]
Test to ensure we don't need to reload an image in a document

Ok, here's a mochitest for this bug. I load a big (3000x3000) PNG file, then load another image, forcing the image cache to try to evict any elements it can. Then we do the same trick that the eBay page does - img = new Image(), then img.src = big.src. If immediately after that we don't have a width, the image wasn't able to be loaded from cache. (Note: I know I claimed that I was OK with breaking this, and it might seem a little hypocritical to create a test that will ensure we don't break it in future given that stance. I only said that because I thought it would be hard to fix. :) )

Anyways, the reason I flag you for review on this, Boris, is because I tried to roll another test into this test - namely, one that would test imgCacheValidator. Unfortunately, as I discovered, if you're reloading the same image from the same document (the aCX parameter passed to LoadImage()), the image cache just assumes all's well and doesn't validate. It takes an explicit reload to get imgCacheValidator involved. Is this correct? If so, should I just do a document.reload() in a separate test?
Comment 31 Joe Drew (not getting mail) 2009-01-22 15:56:08 PST
Forgot to mention: current trunk fails on this mochitest, and trunk + attachment 357879 [details] [diff] [review] passes.
Comment 32 Boris Zbarsky [:bz] 2009-01-22 20:32:36 PST
I have no clue where to start in on the validator stuff.  I don't even know the answer to the question in comment 30.

Is there any chance of getting Stuart to review this?  Other than maybe darin he's the only one who has a shot of already knowing how the validator thing works.  I'd have to sit down and sort through it.

If it makes him feel better, I'd rather do interruptible reflow stuff than sort though imgCacheValidator, but this bug is a blocker.... ;)
Comment 33 Stuart Parmenter 2009-01-23 13:26:19 PST
Comment on attachment 357879 [details] [diff] [review]
Address review comments

this looks OK.
Comment 34 Joe Drew (not getting mail) 2009-01-23 15:23:35 PST
Created attachment 358498 [details] [diff] [review]
Fix leaks in previous patch

As is usually the case with my patches :), I found a leak or two in the previous attempt. Namely, removing an entry that didn't make it into the cache (because it failed to initialize) didn't work, even though it was in the tracker and queue. I fixed this by changing RemoveEntry(imgCacheEntry *) to directly remove entries rather than relying on removing by URI.

Also, I forgot to change EvictEntries() to account for the fact that the cache now has a superset of things in the queue, so on XPCOM shutdown not everything got freed.

I'm carrying over Stuart's r+, but I'd like Vlad to look this over.
Comment 35 Vladimir Vukicevic [:vlad] [:vladv] 2009-01-23 15:53:15 PST
Comment on attachment 358498 [details] [diff] [review]
Fix leaks in previous patch

Looked at diff; looks fine to me.
Comment 37 Joe Drew (not getting mail) 2009-01-26 14:19:28 PST
Backed out because of leaks :(
Comment 38 Joe Drew (not getting mail) 2009-01-30 16:04:41 PST
Created attachment 359850 [details] [diff] [review]
Fix obscure leak by checking for eviction

In certain obscure cases, such as are triggered in the mochitest for bug 368972, it was possible for an imgRequest to lose its proxies after it was cancelled. (I'm kind of surprised that no other tests triggered that.) Since it makes no sense for the cache to have a queue entry for elements that aren't in the cache's hash table, we can just skip out if the entry has been evicted.
Comment 39 Joe Drew (not getting mail) 2009-01-30 20:45:50 PST
http://hg.mozilla.org/mozilla-central/rev/ff62653d0cd8 and http://hg.mozilla.org/mozilla-central/rev/380f8cf4bec2 . Looks to be sticking this time.
Comment 41 Stephen Donner [:stephend] 2009-01-31 13:35:12 PST
Verified FIXED on trunk using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090129 Minefield/3.2a1pre; thanks Joe!
Comment 42 Stephen Donner [:stephend] 2009-02-01 12:24:41 PST
Verified FIXED on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b3pre) Gecko/20090201 Shiretoko/3.1b3pre; replacing fixed1.9.1 keyword with verified1.9.1
Comment 43 IU 2009-02-02 02:01:49 PST
I suspect this is responsible for the following crashes in Core (happening both with Minefield and Shredder):

Shredder (during application shutdown):
http://crash-stats.mozilla.com/report/index/5b7ab37b-7d7b-4e14-aef1-5d0d12090201
http://crash-stats.mozilla.com/report/index/a7affa2d-f083-4b34-ad5d-aca0d2090202

Minefield:
http://crash-stats.mozilla.com/report/index/79eea9ca-ce37-46c6-a9d0-3bc1c2090201

First reported here: http://forums.mozillazine.org/viewtopic.php?p=5642865#p5642865
Comment 44 Bob Clary [:bc:] 2009-02-02 05:53:17 PST
filed bug 476457 for the crashes.
Comment 45 Vladimir Vukicevic [:vlad] [:vladv] 2009-02-02 21:23:39 PST
Reopened due to 476457. :(
Comment 46 Vladimir Vukicevic [:vlad] [:vladv] 2009-02-02 21:24:26 PST
Er, and bug 476349.
Comment 47 Joe Drew (not getting mail) 2009-02-10 19:03:41 PST
Created attachment 361697 [details] [diff] [review]
Check the return value of nsExpirationTracker::AddObject

I can't guarantee that this fixes all crashes on shutdown, but it definitely fixes a crash I was seeing on shutdown when creating a new profile, and it's a smart thing to do. Basically, sometimes it was possible for nsExpirationTracker::AddObject to fail, but we didn't check for that possibility. Now we handle it gracefully.
Comment 48 Vladimir Vukicevic [:vlad] [:vladv] 2009-02-11 01:34:49 PST
I just threw this patch over to the try server -- see if the people who were seeing the crashes frequently can reproduce with it?
Comment 49 Joe Drew (not getting mail) 2009-02-11 13:45:11 PST
Here is a link to Vlad's try server builds. Please test with these and let us know whether you see this crash.

https://build.mozilla.org/tryserver-builds/2009-02-11_01:34-vladimir@mozilla.com-bug466586/
Comment 50 Joe Drew (not getting mail) 2009-02-13 08:37:49 PST
After careful consideration, I'm going to say that we can't release B3 without this. The patch is scary enough that it really needs a large audience of testers - i.e., a beta vehicle. I'm comfortable pushing this to mozilla-central and letting it bake for a few days right ATM; we can reevaluate this later if it's necessary.
Comment 51 Joe Drew (not getting mail) 2009-02-13 14:25:31 PST
http://hg.mozilla.org/mozilla-central/rev/52da9e8d713a and http://hg.mozilla.org/mozilla-central/rev/af9270b650e6

Going to let this bake in m-c for several days to ensure that all the nits have been shaken out.
Comment 52 Joe Drew (not getting mail) 2009-02-13 15:29:16 PST
Backed out again because of a strange crash: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1234564770.1234566952.27920.gz
Comment 53 Joe Drew (not getting mail) 2009-02-17 16:31:49 PST
http://hg.mozilla.org/mozilla-central/rev/a6b38a1f6624 and http://hg.mozilla.org/mozilla-central/rev/c48c9cc92320. Looks like there's no obvious failures this time, but I have got to believe that there's a bug lying dormant because of the crash mentioned in comment 52. I hope not to back this out; please file follow-up bugs for crashes/assertions you find.
Comment 54 Stephen Donner [:stephend] 2009-02-19 11:19:33 PST
Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090219 Minefield/3.2a1pre; I had a harder time reproducing the failure this time around than when I filed the bug, but did with a two-day old build, using Martijn's testcase:

https://bugzilla.mozilla.org/attachment.cgi?id=352339

That now works with the above build, in which I left it running for a long time.
Comment 55 neil@parkwaycc.co.uk 2009-02-20 02:10:27 PST
Created attachment 363282 [details] [diff] [review]
Related crash stack?

I was trying to type into a textarea at the time, although I get the same stack on similar-looking crashes closing windows.
Comment 57 Stephen Donner [:stephend] 2009-03-24 10:34:44 PDT
Verified FIXED in 1.9.1 using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b4pre) Gecko/20090324 Shiretoko/3.5b4pre

Replacing fixed1.9.1 keyword with verified1.9.1
Comment 58 Ben Newman (:bnewman) (:benjamn) 2009-04-24 15:11:43 PDT
(In reply to comment #30)
> Created an attachment (id=358292) [details]
> Test to ensure we don't need to reload an image in a document

This seems to be failing intermittently:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1240597768.1240608481.27987.gz&fulltext=1#err7

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1240581329.1240591791.5433.gz&fulltext=1#err7

> (Note: I know I claimed that I was
> OK with breaking this, and it might seem a little hypocritical to create a test
> that will ensure we don't break it in future given that stance. I only said
> that because I thought it would be hard to fix. :) )

Given that it seems tricky to test this reliably, what are your current feelings?
Comment 59 Joe Drew (not getting mail) 2009-04-24 16:41:36 PDT
The fact that this fails intermittently means there's something wrong with the code, not with the test. :(
Comment 61 Matthew Gregan [:kinetik] 2009-05-05 00:48:10 PDT
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1241499935.1241509041.8931.gz
WINNT 5.2 mozilla-central unit test on 2009/05/04 22:05:35
Comment 62 Joe Drew (not getting mail) 2009-05-05 08:27:51 PDT
I've filed bug 490384 on the intermittent failures.
Comment 63 Joe Drew (not getting mail) 2009-05-08 13:15:53 PDT
*** Bug 324405 has been marked as a duplicate of this bug. ***
Comment 64 Joe Drew (not getting mail) 2009-09-03 11:29:25 PDT
*** Bug 301279 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.