102585 - Trunk N622 M099 Crash when linking to a background image that is not available [@ nsImageLoader::Load]

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[TucsonTester2]

Build ID: 20011001

If you choose a picture file, that is not available, to be a background Composer
will crash.

Reproducible: Always

Steps to Reproduce:
1.Open Composer
2.Click on Format - Page Colors and Background
3.Type in any text for the background image file (make sure you add something
like .gif) and click ok

Actual Results:
Composer will crash

Expected Results:
I would expect that composer would not crash.  Instead I would expect that
composer would not display a background image and add the html to link to the
image file I specified.

Comment 1

[TucsonTester2]

1.Open Composer
2.Click on Format - Page Colors and Background
3.Type in any text for the background image file (make sure you add something
like .gif) and click ok
4.Click on the Preview or Normal tab to switch views

After following these steps composer will crash.  If you click on the "Show All
Tags" or "HTML Source" view nothing will happen.

Comment 2

[Ashley Bischoff (blog at handcoding.com)]

Reporter: Please try a talkback-enabled build:
http://ftp.mozilla.org/pub/mozilla/nightly/latest/mozilla-win32-talkback.zip

(Or, if you're not using Windoze, please use this link:
http://ftp.mozilla.org/pub/mozilla/nightly/latest/ )

Then, if you get a crash, please post the Talkback ID here.

Comment 3

[TucsonTester2]

Another comment for above: You may have to switch views a few times but
eventually composer will crash.

Another way to get this to reproduce is to follow the steps below:

1.Open Composer
2.Click Format -> Page Colors and Background
3.Click on "Choose File" and pick a file for a background image
4.Click save and save the page in the same folder as the background image
5.Click on Format -> Page Colors and Background
6.Click on "url is relative to page location" option and click ok

Once you have folowed the steps composer will crash.


Additional Info:
If you open a page with a background image that does not have a relative link
and then make it relative composer will not crash.  It is only when you first
insert the backgrond image that it will cause composer to crash.

I'm using Mac OS X and there is not a talkback enabled build. But, I got someone
to test it on Win 2k and here is the talkback id number they received.

Talkback ID: TB36133408Z

Comment 4

[Ashley Bischoff (blog at handcoding.com)]

CCing Asa for talkback retieval, please.

Comment 5

[Asa Dotzler [:asa]]

Incident ID 36133408
Stack Signature nsImageLoader::Load 4445a0f7
Bug ID
Trigger Time 2001-10-01 14:57:01
Email Address
User Comments I had just added a link to a background image that was not
currently in the specified directory and applied the change.
Build ID 2001092805
Product ID Netscape6.20
Platform ID Win32
Trigger Reason Access violation
Stack Trace
nsImageLoader::Load
[d:\builds\seamonkey\mozilla\layout\base\src\nsImageLoader.cpp, line 113]
nsPresContext::LoadImage
[d:\builds\seamonkey\mozilla\layout\base\src\nsPresContext.cpp, line 1201]
nsCSSRendering::PaintBackground
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSRendering.cpp, line 2229]
nsHTMLContainerFrame::Paint
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsHTMLContainerFrame.cpp, line 86]
CanvasFrame::Paint
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsHTMLFrame.cpp, line 401]
PresShell::Paint
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5436]
nsView::Paint [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 275]
nsViewManager::RenderDisplayListElement
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 1441]
nsViewManager::RenderViews
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 1366]
nsViewManager::Refresh [d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp,
line 901]
nsViewManager::DispatchEvent
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 1964]
HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 68]
nsWindow::DispatchEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 732]
nsWindow::DispatchWindowEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 754]
nsWindow::OnPaint [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp,
line 4071]
nsWindow::ProcessMessage
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3054]
nsWindow::WindowProc
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 997]
USER32.DLL + 0x3eb0 (0x77e13eb0)
USER32.DLL + 0x591b (0x77e1591b)
USER32.DLL + 0x595d (0x77e1595d)
ntdll.dll + 0x1fb83 (0x77f9fb83)
USER32.DLL + 0x92da (0x77e192da)
nsAppShellService::Run
[d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsAppShellService.cpp, line 468]
netscp6.exe + 0x174b (0x0040174b)
netscp6.exe + 0x121a (0x0040121a)
netscp6.exe + 0x3428 (0x00403428)
KERNEL32.DLL + 0x7903 (0x77e87903) 

Comment 6

[greer]

Adding the signature to the summary to help talkback track this one. I will hold 
off on adding keywords - talkback data is inconclusive as to whether the Tucson 
tester is the only crasher.

Comment 7

[TucsonTester2]

Just reproduced this on Mac OS 8.6 using the 20011001 build.

Talkback ID: TB36167729X

Comment 8

[greer]

Here's the talkback report:

 Incident ID   :36167729 
 Bug ID
 Trigger Time  :2001-10-02 11:34:02 
 User Comments :I was clicking ok on the Advanced Property editor after changing 
the background image to a file that was not currently in the directory 
specified. 
 Build ID      :2001100103 
 Platform ID   :MacOS 
 Trigger Reason:PowerPC unmapped memory exception 

 Stack Trace   :

0x78003968 
nsPresContext::LoadImage() [nsPresContext.cpp, line 1206] 
nsCSSRendering::PaintBackground() [nsCSSRendering.cpp, line 2226] 
nsHTMLContainerFrame::Paint() [nsHTMLContainerFrame.cpp, line 82] 
CanvasFrame::Paint() [nsHTMLFrame.cpp, line 399] 
PresShell::Paint() [nsPresShell.cpp, line 5438] 
VIEW_DLL + 0x597c (0x17b8c38c) 
VIEW_DLL + 0x11670 (0x17b98080) 
VIEW_DLL + 0x1145c (0x17b97e6c) 

Comment 9

[greer]

Marking this a topcrash. The incidents are too few to make the top40 reports for
each release but there are 15 incidents with this signature spread over M094,
N620 and Trunk from multiple users. (The second crash that the reporter logged
was on Mac - same stack but different top frame, so there may be more of these
hiding.)

Here are the user comments:
2001092605 (36222888) Windows 98  4.90 build 73010104 composer was doin some
funky stuff  (dont know exactly what I did to cause the crash)

2001092805 (36133408) Windows NT  5.0 build 2195 I had just added a link to a
background image that was not currently in the specified directory and applied
the change. (reporter's comment)

2001100105 (36131549) Windows 98  4.10 build 67766222 NORMAL PREVIEW CRASHES
2001100105 (36132471) Windows 98  4.10 build 67766222 FORMAT COLORS TEXT AND
BACKGROUND CRASHES



M094 crashes:
 (35958617) Windows NT  5.0 build 2195 I was designing a web page.  I was adding
the background image
 (36231813) Windows 98  4.10 build 67766222 while using composer.i copied the
source code from a website and pasted it into composers html view.then went to
normal and deleted a couple of sections and went to tag view or such.Now when i
clicked back to normal view  it crashed.
 (36231884) Windows 98  4.10 build 67766222 just reproduced the same
problem...crash when going back to normal view from tag view.
 (36069878) Linux 2.4.3-20mdk Editing a HTML Page ...

Comment 10

[greer]

Just now on my NT4 box i was able to get the browser to hang and the sreen
failed to repaint. 
1. went to espn.com 
2. copied the source of the home page from <body> to </body> and pasted it into
composer
3. switched back and forth between source, preview and normal, no crash.
4. select all, deleted the HTML code in composer
5. Selected all of the espn.com HTML source
6. Tried to paste and the composer window hung.

Comment 11

[Syd Logan]

Reassigning layout

Comment 12

[Kevin McCluskey (gone)]

Attachment: Patch which fixes the crash but the base URL needs to be set properly

Comment 13

[Kevin McCluskey (gone)]

The problem is the base URI is set to about:blank. 

If the document is saved it will NOT crash when switching between modes.

I attached a patch that looks for a null URI in nsImageLoader::Load (which is a
reasonable check to do), but the underlying cause which is the bad base URI
(about:blank) should also be fixed.  

If only the patch which prevents the crash is checked in, the composer window
will show garbage when switching between modes because the body is transparent.

Kin, can you look at setting the base URI to something other than about:blank
for a new composer document?

Comment 14

[kinmoz]

kmcclusk, I'm not sure we want to set the URL to something else, after all the 
doc isn't anywhere but in memory.

Cc'ing cmanske and brade for their opinion. I know that we have editor code that 
keys off of the fact that the url for the current doc is about:blank.

I didn't have time to really look into this milestone, do we want to just 
checkin the null-check to prevent the crash for now?

There are several approaches I can think of to avoid the assertions:

1. Set the doc URL to some dummy http url that composer recognizes and
   keys off of.

2. Store the image relative URLS for unsaved-new docs as dummy http urls for
   example "http://composer-unsaved-doc-relative-url-placeholder/myimage.gif"
   and then strip them when saved/published.

3. Modify layout to handle special case about:blank like everyone else does.

Comment 15

[Charles Manske]

Yea, the "new doc" state is a real pain! 
Note that we don't force using a valid image extension, so this problem will 
occur more frequently now :(
I'd favor option 3 that kin suggests in #14

Comment 16

[Kevin McCluskey (gone)]

Regardless, I think we should check in my original patch,  since the result of
NS_NewURI can be a null url (as is the case when you have a bad base URI).

Although it is sorta Kludgy to have layout check for about:blank when forming a
relative URI for a image I guess we could put checks into Layout as long as we
wrap it in a Method like IsValidBaseURI and the implemention of IsValidBaseURI
looks for about:blank. 

Comment 17

[Kathleen :Brade]

I am opposed to specifying "http:" protocol for documents which are now
"about:blank".  I'd also be opposed to specifying the "file:" protocol.  Let's
just stick with the "about:blank" url.

Sounds like Kevin's plan above is on track.

Comment 18

[kinmoz]

Giving back to kmcclusk so he can check in his null-check patch.

Comment 19

[kinmoz]

Comment on attachment 59252 [details] [diff] [review]
Patch which fixes the crash but the base URL needs to be set properly

sr=[e-mail] on the null-check patch.

kmcclusk and I talked about the fact that we need to bail before we fire off
the load to avoid the assertions and possible problems down the line. He's
going to checkin the null-check for now to prevent the crash since we should be
checking for null there anyways.

Comment 20

[Marc Attinasi]

Comment on attachment 59252 [details] [diff] [review]
Patch which fixes the crash but the base URL needs to be set properly

r=attinasi

Comment 21

[Marc Attinasi]

Sorry if I'm jumping in too late here, but why do we use about:blank as the base
URI for an unsaved document? Wouldn't it be better to start out with a legit
temp file and use that as the base URI? You could even save to that temp file
periodically, for crash-recovery. I know that is more work, but maybe it adds
some value...

> 3. Modify layout to handle special case about:blank like everyone else does.
What does this mean? If about:blank is the base URI, should we create a URI like
'about:myimage.png'? That could be interesting! Who else besides Composer is
special-casing the about:blank URI?

I get the impression that the desire to stick with the about:blank is related to
the effort required to change it, not to its correctness. That said, the 'about'
protocol could be made to act as a base URI I guess.

r=attinasi for the null check :)

Comment 22

[Kevin McCluskey (gone)]

Fix for crash checked in, crash, topcrash removed. Keeping the bug open for
further cleanup.

Comment 23

[greer]

Kevin, we need the "crash" and "topcrash" keywords for tracking in talkback. 
Replacing. 

Comment 24

[Kevin McCluskey (gone)]

Bulk moving Mozilla1.1 bugs to future-P2. I will pull from this list when
scheduling post Mozilla1.0 work.

Comment 25

[Shiva Thirumazhusai]

nominating topcrash bugs for nsbeta1. 

Comment 26

[Kevin McCluskey (gone)]

nsbeta1-. It no longer crashes.

Comment 27

[greer]

Kevin, the crash does indeed look fixed per Talkback data. You mention further
cleanup in Comment #22, but the bug is for the crash. Can you log another bug
and resolve this one? Thanks. 

Comment 28

[greer]

I was going to close out this topcrasher as fixed, but I looked at the current
data in M099, N621 and N622 and still see this signature with comments that look
like users are having the same problem. The fix went in mid-December, we should
be seeing these now. perhaps we need more than the null check.

N621 (nsImageLoader::Load):       33
(4375825) - [Windows NT  5.1 build 2600]: Adding a background image to a login
page while using "Composer".  
(4375896) - [Windows 98  4.90 build 73010104]: I was trying to save a web page I
was writing in the HTML editor. 
(4404536) - [Windows 98  4.90 build 73010104]: Composing. Had added attributes
to the <body> tag in html source mode  crash occurred 
when switching back to normal editing mode. 
(4430249) - [Windows 98  4.10 build 67766222]: saving a composer file  
(4435940) - [Windows NT  5.1 build 2600]: I was using Netscape Composer.I have
changed in the HTML Source the path of the background file and I have tried to
save with the save button.  
(4681557) - [Windows 98  4.90 build 73010104]: checking out web pages at
migente.com URL: www.migente.com
(4688839) - [Windows 98  4.90 build 73010104]: hey cool hp! URL: http://www.web.de
(4688922) - [Windows 98  4.90 build 73010104]: hey cool hp! URL: http://www.web.de
(4677798) - [Linux 2.4.17]: Changing a font color URL: www.dream-tools.com
 
N622 (nsImageLoader::Load):       15
(4390059) - [Windows 98  4.10 build 67766446]: applying a background to a web page  
(4405800) - [Windows NT  5.0 build 2195]: Using composer for a page with style
sheets which control mousover popup which renders with IE and Netscape 4.7 and
Opera  but not with navigator 6 2 
(4460369) - [Windows 98  4.10 build 67766222]: I was using composer...putting in
a background image. 
(4463835) - [Windows NT  5.1 build 2600]: Making a WebSite 
(4533338) - [Windows NT  5.0 build 2195]: Previewing a very new web page I was
making. 
(4539314) - [Windows 98  4.10 build 67766446]: adding a jpeg as background and
netscape crashed URL: using composer
(4587773) - [Windows 98  4.10 build 67766446]: choosing the preview option URL:
in netscape composer
(4721007) - [Windows 98  4.90 build 73010104]: Uploading web site 
(4721271) - [Windows NT  5.1 build 2600]: Also had Composer open. Have been
having trouble everday with crashing while using browser.
 URL: http://www.jpost.com/
 
M099 (nsImageLoader::Load):        2
(4584382) - [Windows NT  5.0 build 2195]: Was going to wamu website.typed
"washingtonmutual" in the address barAfter navigation  I used my "enter" key to
dismiss the cookie and image boxes that appeared (I have the prompt for cookies
and images settings turned on) URL: http://www.wamu.com/servlet/wamu/index.html

I've tried a few of the comments to reproduce the crash but have been
unsuccessful thus far.

Comment 29

[Kevin McCluskey (gone)]

This fix was NOT checked in for N6.2 N6.21. 

It was checked in before M099, but the crash you show for M099 does not appear
to be the same problem. The M099 crash is in the address bar. Not composer.

Comment 30

[Kevin McCluskey (gone)]

Cleanup is covered in http://bugzilla.mozilla.org/show_bug.cgi?id=134996

Comment 31

[greer]

Attachment: Stack for incident #5167162

Kevin, Trunk builds in the last week have a few crashes that appear to be the
same problem. 

Trunk (nsImageLoader::Load):	    3
 Unique Users  3
(5167162) - [2002040910 - 2002-04-13] Crashed while loading images...Not sure
exactly why. URL: http://www.remotelyanywhere.com/ Windows 98  4.10 build
67766446
(5147036) - [2002040910 - 2002-04-12]  URL: www.wamu.com  Windows 98  4.10
build 67766446

Looking at the first crash listed above. It is crashing at line 103 (Talkback
line no.s are one off on Win builds) LXR:

102 nsCOMPtr<nsIDocument> doc;
103 rv = shell->GetDocument(getter_AddRefs(doc));
104 if (NS_FAILED(rv)) return rv;

The values when the crash happened were:
nsImageLoader::Load
this = Register variable - data not available	     
aURI = 0x026d47b8	 
   (*aURI) = Data not available        
baseURI = {nsCOMPtr<nsIURI>}	    
loadGroup = {nsCOMPtr<nsILoadGroup>}	    
doc = {nsCOMPtr<nsIDocument>}	     
il = {nsCOMPtr<imgILoader>}	   
shell = {nsCOMPtr<nsIPresShell>}   <-- Looks like shell is null       
rv = 0 (0x00000000)	   
uri = {nsCOMPtr<nsIURI>}	
oldURI = {nsCOMPtr<nsIURI>}	   
eq = 1242684 (0x0012f63c)

Looks like we need a check to see if a member value is null in GetShell. 

Though in
http://lxr.mozilla.org/mozilla/source/layout/base/src/nsPresContext.cpp#737
it looks like it should always be returning NS_OK.

737 nsPresContext::GetShell(nsIPresShell** aResult)
738 {
739 NS_PRECONDITION(aResult, "null out param");
740 *aResult = mShell;
741 NS_IF_ADDREF(mShell);
742 return NS_OK;   <-- Always true?
743 }

Any ideas on this one?

Comment 32

[Kevin McCluskey (gone)]

This latest crash is really a separate bug from the original crash. The original
crash only occurred when the user was in composer and they linked to a
background image and they did not have their document stored. The only
commonality is (nsImageLoader::Load.  I would prefer to open a new bug. Its too
confusing to read through the first part of the bug which no longer applies.

Comment 33

[Chris Waterson]

So shiva showed me this crash.

It looks like pavlov needs to check the out-param from nsPresContext::GetShell
instead of checking the return code, which is always NS_OK. (Shame on me, I
sr='d this.) So that's a good fix to just get the API semantics right in
nsImageLoader.cpp. Since the PresShell calls SetShell(nsnull) on the context
from its Destroy() method, it's not unthinkable that the PresContext won't have
a shell to cough up.

Looking a bit deeper, perhaps we ought not to paint (i.e., PresShell::Paint
ought to just bail) once the PresShell has been destroyed. What do you think, Kevin?

Comment 34

[Kevin McCluskey (gone)]

Yes, I agree that we should abort the paint if the PresShell is destroyed. But
I'm wondering what happened to cause the PresShell to be destroyed as a result
of processing the paint earlier in the stack? The PresShell dispatched the paint
to canvas frame so it was around earlier in the stack and then the PresShell is
null when the imageLoader references it through the PresContext?. 

nsImageLoader::Load
[d:\builds\seamonkey\mozilla\layout\base\src\nsImageLoader.cpp, line 104]
nsPresContext::LoadImage
[d:\builds\seamonkey\mozilla\layout\base\src\nsPresContext.cpp, line 1544]
nsCSSRendering::PaintBackgroundWithSC
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSRendering.cpp, line 2752]
nsCSSRendering::PaintBackground
[d:\builds\seamonkey\mozilla\layout\html\style\src\nsCSSRendering.cpp, line 2634]
nsHTMLContainerFrame::Paint
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsHTMLContainerFrame.cpp, line 98]
CanvasFrame::Paint
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsHTMLFrame.cpp, line 390]
PresShell::Paint
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5775]

Comment 35

[Kevin McCluskey (gone)]

It should be an invariant that during paint the presshell, prescontext, and
frames must not be destroyed. If this happens we may crash in other areas since
the destroyed frame or the frame's presshell may be referenced as the result of
the  viewmanager processing the rest of the display list.  We may want to add an
NS_ASSERTION to the destroy method of presshell to check to see if we are
currently painting. This may help us track down this abbarant case. Something like:


PresShell::Destroy()
{
    ...

#ifdef DEBUG
    nsCOMPtr<nsIViewManager> viewManager;
    nsresult rv = GetViewManager(getter_AddRefs(viewManager));
    if (NS_SUCCEEDED(rv) && (nsnull != viewManager)) {
      PRBool isPainting = PR_FALSE;
      viewManager->IsPainting(isPainting);
      NS_ASSERTION(! isPainting, "We are destroying the PresShell while painting");
#endif

Comment 36

[Kevin McCluskey (gone)]

There seem to only be three cases where nsPresContext's mShell would be null:

1. nsPresContext::SetShell(nsnull) was called from the PresShell::Destroy()
2. nsPresContext was created but nsPresContext::SetShell was never called
3. nsPresContext was destroyed. 

Comment 37

[Kevin McCluskey (gone)]

If I had to guess I would say that an error in the refcounting of the
nsPresShell is the real bug. During the processing of the paint it may be doing
a NS_RELEASE on a nsPresShell instance with an incorrect refcount which triggers
the destruction of the nsPresShell.

Comment 38

[Kevin McCluskey (gone)]

This is interesting.. may be related. Even though the PresShell is "kungFu death
gripped" in UnsuppressAndInvalidate() it still results in the PresShell being
destroyed as the result of calling cv->Show(); according to the comment in the code.

PresShell::UnsuppressAndInvalidate()

...

   if (cv) {
     nsCOMPtr<nsIPresShell> kungFuDeathGrip(this);
     cv->Show();
     // Calling |Show| may destroy us.  Not sure why yet, but it's
     // a smoketest blocker.
     if (mIsDestroying)
       return;
     }
    ...

Comment 39

[Kevin McCluskey (gone)]

Note: The nsPresShell::Destroy method must have been called after
nsPresShell::HandleEvent received the paint message and passed the paint down to
the container frame because there is code at the beginning of the PresShell
which would have aborted the processing of the paint if the presshell had been
in the mIsDestroying state when the paint was dispatched.

PresShell::HandleEvent(nsIView...)
  ...

  if (mIsDestroying || mIsReflowing) {
    return NS_OK;
  }

Comment 40

[Kevin McCluskey (gone)]

Disregard my last comment. I was looking at the line numbers in the stack trace
and the nsPresShell has changed enough since the stack was posted what I stated
is false. It may be the case that the preshell was destroyed before the paint
message was dispatched. Which is probably the most likely case.

Comment 41

[Kevin McCluskey (gone)]

Attachment: Abort paint if destroying the PresShell and check for null PresShell in nsImageLoader

Comment 42

[Kevin McCluskey (gone)]

waterson: Chris can you super review the patch (id=81160)? It is basically what
you described as the solution in comment #33 

Comment 43

[Marc Attinasi]

Comment on attachment 81160 [details] [diff] [review]
Abort  paint if destroying the PresShell and check for null PresShell  in nsImageLoader

r=attinasi

Comment 44

[Chris Waterson]

Comment on attachment 81160 [details] [diff] [review]
Abort  paint if destroying the PresShell and check for null PresShell  in nsImageLoader

sr=waterson

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 81160 [details] [diff] [review]
Abort  paint if destroying the PresShell and check for null PresShell  in nsImageLoader

a=dbaron for 1.0 branch checkin, although

NS_ASSERTION(PR_FALSE, "...")

could be written as:

NS_NOTREACHED("...")

Comment 46

[scottputterman]

adding adt1.0.0+.  Please check into the branch as soon as possible and add the
fixed1.0.0 keyword.

Comment 47

[Kevin McCluskey (gone)]

Fix checked into the trunk and Mozilla1.0.0 branch.

Comment 48

[sujay]

verified in 4/29 trunk build.

Comment 49

[sujay]

verified in 4/29 branch build.