Bug 56494 - Uninitialized variable "useFallback" in WebCore/loader/SubframeLoader.cpp
Summary: Uninitialized variable "useFallback" in WebCore/loader/SubframeLoader.cpp
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-16 15:25 PDT by asharif.tools
Modified: 2011-04-14 10:42 PDT (History)
3 users (show)

See Also:


Attachments
This patch initializes the usefallback variable. (1.52 KB, patch)
2011-03-16 15:30 PDT, asharif.tools
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description asharif.tools 2011-03-16 15:25:41 PDT
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.
Comment 1 asharif.tools 2011-03-16 15:30:32 PDT
Created attachment 85989 [details]
This patch initializes the usefallback variable.
Comment 2 Alexey Proskuryakov 2011-03-17 10:35:32 PDT
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.
Comment 3 Nate Chapin 2011-03-17 11:16:44 PDT
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;'
Comment 4 Alexey Proskuryakov 2011-03-17 11:23:19 PDT
Nate, would you like to work on that? I guess the most tricky part would be figuring out if a test can be made.
Comment 5 Nate Chapin 2011-03-17 11:30:05 PDT
(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.
Comment 6 Nate Chapin 2011-03-24 13:27:41 PDT
(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.
Comment 7 Alexey Proskuryakov 2011-03-24 13:45:15 PDT
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>
Comment 8 Nate Chapin 2011-03-24 16:00:59 PDT
(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"
Comment 9 Alexey Proskuryakov 2011-03-24 16:57:27 PDT
Did you try installing the Acordex plug-in? My guess is that would get you to this code path.
Comment 10 Nate Chapin 2011-04-14 10:42:38 PDT
(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?