Bug 56494 – Uninitialized variable "useFallback" in WebCore/loader/SubframeLoader.cpp

• Component: WebCore Misc.
• Status: NEW

There is an uninitialized variable "useFallback" in file WebCore/loader/SubframeLoader.cpp at lines around 96 and 116.

I'll attach a patch that fixes it.

Created attachment 85989 [details]
This patch initializes the usefallback variable.

[Alexey Proskuryakov]

useFallback is a result of shouldUsePlugin() function, it needn't be initialized by the caller.

But there appears to be an issue here nonetheless - shouldUsePlugin() doesn't assign anything to useFallback in some code paths! This seems quite bad, CC'ing folks who were the last to touch this code.

[Nate Chapin]

Comment on attachment 85989 [details]
This patch initializes the usefallback variable.

I just checked the revision logs, looks like this is an old bug that I moved from FrameLoader to SubframeLoader while refactoring.  Sorry for not catching it then :(

We should probably ensure that shouldUsePlugin() sets useFallback on all paths.  It looks like the early exit at http://trac.webkit.org/browser/trunk/Source/WebCore/loader/SubframeLoader.cpp?rev=77752#L305 is the only problem, and I think that should be 
'useFallback = false;'

[Alexey Proskuryakov]

Nate, would you like to work on that? I guess the most tricky part would be figuring out if a test can be made.

[Nate Chapin]

(In reply to comment #4)
> Nate, would you like to work on that? I guess the most tricky part would be figuring out if a test can be made.

I can try, though I almost certainly won't get to it before Monday.

[Nate Chapin]

(In reply to comment #5)
> (In reply to comment #4)
> > Nate, would you like to work on that? I guess the most tricky part would be figuring out if a test can be made.
> 
> I can try, though I almost certainly won't get to it before Monday.

I've had no luck triggering this code path at all.  It was introduced in http://trac.webkit.org/changeset/28451, which references to rdar://problem/5602071.  Perhaps there are repro steps there?

Alternatively, I'm happy to upload my existing patch to initialize the variable in all paths of shouldUsePlugin() if that satisfies everyone.

[Alexey Proskuryakov]

There was a cluster of related bugs that were saying one of the two things:
1) Scanned 300dpi patent pages on uspto.gov are displayed too large. Looking at steps to reproduce, I see that Safari 5.0.4 fails to load these on my machine for an unrelated reason, and Firefox crashes. Uh-oh.
2) Plugin from http://acordex.com/ wasn't used on these pages if installed.

None of these seemed to have anything to do with fallback. The markup looked something like:

<embed src="/.DImg?Docid=06702398&PageNum=1&IDKey=966709C39671&ImgFormat=tif" width="570" height="840" type=image/tiff></embed>

[Nate Chapin]

(In reply to comment #7)
> There was a cluster of related bugs that were saying one of the two things:
> 1) Scanned 300dpi patent pages on uspto.gov are displayed too large. Looking at steps to reproduce, I see that Safari 5.0.4 fails to load these on my machine for an unrelated reason, and Firefox crashes. Uh-oh.
> 2) Plugin from http://acordex.com/ wasn't used on these pages if installed.
> 
> None of these seemed to have anything to do with fallback. The markup looked something like:
> 
> <embed src="/.DImg?Docid=06702398&PageNum=1&IDKey=966709C39671&ImgFormat=tif" width="570" height="840" type=image/tiff></embed>

Ok, the example I'm using is very similar to that.  

I guess I'm just not sure what to do because I can't even get myself inside the loop starting at http://trac.webkit.org/browser/trunk/Source/WebCore/loader/SubframeLoader.cpp#L301.  I have trouble believing this code is dead now, but shouldUsePlugin() doesn't always get called when a plugin is loaded, and I can't figure out what the precise incantation is to get into shouldUsePlugin() with a mimeType parameter of "image/tiff"

[Alexey Proskuryakov]

Did you try installing the Acordex plug-in? My guess is that would get you to this code path.

[Nate Chapin]

(In reply to comment #9)
> Did you try installing the Acordex plug-in? My guess is that would get you to this code path.

I just got back to this bug and installed the Acordex plug-in.  I can't get Safari to recognize it all, and while Chrome lists it in about:plugins, I haven't gotten Chrome to use this plugin even when quicktime is disabled.  Firefox uses it correctly.

When I installed the plugin, it popped up a message that included "Safari versions 3 and later currently do not allow plug-ins to handle images, so we cannot modify Accel ViewTIFF to work with these versions of Safari."  Any idea what's up with that?