Closed Bug 236313 Opened 21 years ago Closed 20 years ago

Mozilla crashes after clicking "SKYPASS" at Korean Air site

Categories

(Core :: Graphics: ImageLib, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: v.kazmirenko, Assigned: tor)

References

()

Details

(Keywords: crash, testcase, verified1.7)

Attachments

(4 files, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113

Open http://www.koreanair.com/
At top of page there is link "SKYPASS". Click it. After opening this page
Mozilla crashes.


Reproducible: Always
Steps to Reproduce:
1.
2.
3.
Summary: Mozilla crashes after clicking "SKYPASS" and opening page (URL is provided above) → Mozilla crashes after clicking "SKYPASS" at Korean Air site
confirming crash FF 20040302 Win2k, I get a popup R0625, Pure Virtual Call.
Keywords: crash, stackwanted
Confirming "R0625, Pure Virtual Call".
This is on Gecko/20040113, Windows 2000.

However, the behaviour isn't very reproductible here. The amount of time before
the crash seems quit unconsistent.
Confirming. Could be a dupe of 236196 but it seems to be slightly different.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Also crashes on linux:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040227 Firefox/0.8.0+
Target All platforms based on #4
OS: Windows XP → All
Attached file stack
Bug occurs in 2003-09-03-05 trunk Linux, which is the oldest nightly build
I could find.  Bug does not occur in Mozilla 1.4 on Linux.
Assignee: general → jdunn
Component: Browser-General → ImageLib
Attached file testcase
load the testcase.  reload a few times.  crash
tested trunk build 2004030308

<img src="javascript:SKchoosePhoto()" name="SKbnrPhoto">

SKchoosePhoto is:
  document.SKbnrPhoto.src="resource:///res/samples/Anieyes.gif";
regrsession between linux trunk builds 2003071005 and 2003071122
bug 70030 or bug 212133?
I'll try backing stuff out.
further testing just shows that bug 212133 just got the DOM working used in the
testcase, so no regression.
Keywords: regression
I can't reproduce it on linux with
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040122 Epiphany/1.0.7
and
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040301 Firefox/0.8.0+

(Neither the original page nor the testcase...)
*** Bug 236420 has been marked as a duplicate of this bug. ***
*** Bug 236196 has been marked as a duplicate of this bug. ***
Assignee: jdunn → tor
Attached patch prevent img src collision (obsolete) — Splinter Review
What appears to be happening here is that the src="javascript:..."
load comes in first and kicks off a load.  Deeper down we notice that
it's javascript and run it, which kicks off another load on the same
image, but now with the real URI.  But since the previous load is
still ongoing we pick the wrong request to use (current instead of
pending), which gets overwritten when processing returns to the first
load.
Attached patch prevent img src collision (obsolete) — Splinter Review
Ok, let's attach the patch and not the testcase this time...
Attachment #143546 - Attachment is obsolete: true
Attachment #143547 - Flags: review?(pavlov)
Attachment #143547 - Flags: review?(pavlov)
Attached patch modify both places with test (obsolete) — Splinter Review
Attachment #143547 - Attachment is obsolete: true
Attachment #143722 - Flags: review?(pavlov)
Flags: blocking1.7?
Attachment #143722 - Flags: review?(pavlov) → review+
Attachment #143722 - Flags: superreview?(bzbarsky)
Hmm.. so why is this actually crashing?  Are we double-releasing an
imgRequestProxy or something?

With that patch, won't

function foo() {
  img.src = "javascript:bar()";
}

function bar() {
  img.src = "http://whatever";
}

<img src="javascript:foo()">

still crash?  This time because mPendingRequest is clobbered?
Oh, and no matter what that should be a PRPackedBool and live with the other
PRPackedBools.
Comment on attachment 143722 [details] [diff] [review]
modify both places with test

Yes, the double-indirect thing kills it.
Attachment #143722 - Attachment is obsolete: true
Attachment #143722 - Flags: superreview?(bzbarsky)
*** Bug 235843 has been marked as a duplicate of this bug. ***
*** Bug 240184 has been marked as a duplicate of this bug. ***
Tor, any progress here? This one seems like a good one to try to get for 1.7.
Flags: blocking1.7? → blocking1.7+
*** Bug 242334 has been marked as a duplicate of this bug. ***
Comment on attachment 145337 [details] [diff] [review]
different fix - need to discuss with pavlov

i ran with this patch and i can't find anything that breaks.. go for it
Attachment #145337 - Flags: review+
Attachment #145337 - Flags: superreview?(darin)
Comment on attachment 145337 [details] [diff] [review]
different fix - need to discuss with pavlov

you seem to be transforming the retval of imgILoader::LoadImage into an inout
param.	is that right?	if so, shouldn't you change the IDL accordingly?
Attached patch add [inout] to IDL (obsolete) — Splinter Review
Attachment #145337 - Attachment is obsolete: true
I think we decided on IRC that there is no point to supporting [inout] on the
result of LoadImage (that all callers anyways just want the resulting value and
never pass anything but null).  so, how about just making sure that the
implementation of LoadImage sets *_retval to nsnull before doing anything else.
 that way, the fact that CreateNewProxyForRequest treats _retval as an inout
param will be inconsequential to the caller of LoadImage or LoadImageWithChannel.
To clarify my last comment: whatever we do for LoadImage needs to be done for
LoadImageWithChannel as well.
Attachment #147651 - Attachment is obsolete: true
Comment on attachment 147664 [details] [diff] [review]
tweak per irc discussion

sr=darin

might be good to indicate with a comment the reason why it is significant that
*_retval be nulled out.
Attachment #147664 - Flags: superreview+
Checked in on trunk with comments added.  Leaving open for baking.
I noticed some strange image loading behaviour in a build that I did this morning.
My last build was on 5/3 and I didn't see the bahviour from that build.

I have a homepage with several small images on it which link to larger versions
of the images and they are all local to my hard drive. When I click on one of the
smaller images, the page area is blanked out (white) for a moment before the
image is displayed. Sometimes, this blank time is a fraction of a second and
sometimes it's as long as three seconds.

The image paints very quickly after the pause so it appears that something's going
on that's not related to image painting.

I also noticed this with the MozillaZine png at the top of the page though it
is very short there as the PNG is probably fairly small.

Someone pointed me to this checkin as a possibility.
Note: the behavior described in #34 is not limited to pages with images. On an
internal site I'm developing (which uses only stylesheets) I've noticed that
between the 20040506 zip build and the 20040505 zip build (Windows Firefox),
many pages are MUCH slower on initial load. It seems once they're cached it's fine.

In particular, the pages w/ form controls of any kind, be it text entry boxes,
or form buttons, are noticeable... you can easily see the form component being
drawn before all other page content.

Should another bug be opened about the slow rendering?
This checkin wasn't the cause. I backed it out, did a build and it's still there.
I'll file a bug for now.
Filed bug http://bugzilla.mozilla.org/show_bug.cgi?id=242856 for the other problem that is NOT related to this bug.
Attachment #145337 - Flags: superreview?(darin)
Attachment #147664 - Flags: approval1.7?
Comment on attachment 147664 [details] [diff] [review]
tweak per irc discussion

a=mkaply
Attachment #147664 - Flags: approval1.7? → approval1.7+
Checked in on branch.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Keywords: fixed1.7
Verified as fix on latest 1.7 branch Win06-24, Mac06-30 & Linux06-29 builds.
Changing keywords from fixed1.7 to verified1.7.
Leave this bug status "as is" until this bug be verified on trunk again...
Keywords: fixed1.7verified1.7
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: