444486 - Crash in nsFrame.cpp if BoxMetrics() returns NULL

Status: RESOLVED INVALID

(Core :: Layout, defect)

Description

[John Wolfe (:wolfe)]

Attachment: Check for non-NULL metric pointer before dereferencing

In some circumstances, the function BoxMetrics() can return a NULL pointer.

Fixes to check for non-NULL pointer before using return value.

Comment 1

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

(In reply to comment #0)
> In some circumstances, the function BoxMetrics() can return a NULL pointer.

What circumstances?

We should make it so that all frames for which IsBoxWrapped is true have box metrics.  (We should make nsFrame::Init propagate allocation failure from InitBoxMetrics, and ensure the frame constructor doesn't insert frames for which init fails.)

Comment 2

[John Wolfe (:wolfe)]

I was chasing down issues with a WinMobile Emulator under Windows in Microsoft's Visual Studio 2005.  Seems that the emulator would randomly return a file access denied error when opening files for the first time during an emulator run.

I am no longer certain which file denial resulted in the function BoxMetrics() returning a NULL pointer.  I made this fix a while ago, and am just tonight putting all the fixes I made into patches attached to bugs.

Does this answer your question, or would you like me to try to find which file might be behind the null BoxMetrics() return value?

Comment 3

[Stuart Parmenter]

do we need to do this?

Comment 4

[John Wolfe (:wolfe)]

Well, the exceptions I saw were at-the-edge-of-what-anyone-would-normally-see because you have to have a file access error on one or more files inside the XULRunner bin directory.  

If you are looking to not waste cycles with this bug patch, then might I suggest checking once after the return of the BoxMetrics() function?

Otherwise, if someone is happy to stand up and say "This is not necessary", then at this point, I do not NEED this patch for a WinCE build.  Of course, if the file access problem crops up again (corrupted hard disk, etc.) then the user will just see an unhandled exception.

You tell me, what do people think?

Comment 5

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

We've seen this crash before in situations where we couldn't open core files correctly; however, those are in situations where the browser wouldn't be able to start anyway, so I don't see why this crash is particularly worse than whatever else we would hit in that case.

Comment 6

[John Wolfe (:wolfe)]

Attachment: Same DIFF, just done in Unified instead of Context formatting

Comment 7

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

Comment on attachment 329595 [details] [diff] [review]
Same DIFF, just done in Unified instead of Context formatting

I think the right thing to do here is (1) what I suggested in comment 1 plus (2) bailing somewhere earlier if nsLayoutStylesheetCache::ScrollbarsSheet returns null.

(I suspect file loading errors that would cause nsLayoutStylesheetCache::ScrollbarsSheet to return null are the ones that would trigger this.)

Comment 8

[John Wolfe (:wolfe)]

This bug showed up because the WinMobile Emulators v5.0 and v6.0 were randomly failing when opening files. 

We do not see this error anymore, and a proposed patch I had made was shot down AFTER we figured out that it was the Emulator causing all our troubles.