202817 - Clicking on a link that returns a file of non-trivial mimetype results in an attempt to download rather than view, even if viewing is possible

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GTK Widget, defect, P1)

Description

[Philip Langdale]

This is reproduceable in TestGtkEmbed with both gtk1 and gtk2 embedding (and
galeon, where problem was originally observed).

In the last two weeks or so, something has changed, causing normally handled
mimetypes such as image/pjpeg and application/pdf (with acrobat plugin) to fail
to be handled if links to them are clicked on normally. On the other hand, if
the url is typed into the url box so that |gtk_moz_embed_load_url| is called,
the mimetype is handled in the correct manner.

As such, this appears to be not so much gtkmozembed going wrong, but it failing
to account for some change in backend behaviour that has occured.

This is not simply a failure to recognise plugins, image/pjpeg is handled
internally by mozilla, so there is obviously some core list of mimetypes which
is being referred to to decide on displayability, rather than the comprehensive
list. I seem to recall a hardcoded list of basic mimetypes being used somewhere
in the depths of gecko.

Comment 1

[Boris Zbarsky [:bzbarsky]]

There are several such lists.. but there have been no changes to any of this
code, really, with the exception of bug 201618 (which was not supposed to change
any behavior).

Does backing that patch out fix things for you?  If not, any chance of narrowing
the regression window?

Comment 2

[Philip Langdale]

Well, backing it out does fix the problem... It's obviously a little more than a
reorganisation...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Taking, since this looks like it's mine...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: jpeg to test on

Comment 5

[Boris Zbarsky [:bzbarsky]]

Well, I've figured out the problem... the old code had this hack for getting
helper apps to work -- if no listener were found, it would ask the docshell to
handle the load anyway and then when the docshell bailed it would pass the load
the the helper app service.

I changed this to just go to the helper app service directly if we have no
internal listeners.

This uncovered a bug in the GTK embedding widget, however... In particular, see
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDSURIContentListener.cpp#125
(nsDSURIContentListener::IsPreferred -- this is called when loading via a link
click).  In the embedding case, the parentListener is a EmbedContentListener --
see
http://lxr.mozilla.org/seamonkey/source/embedding/browser/gtk/src/EmbedContentListener.cpp#83

Oops.  That list is kinda short... and should not exist, imo.

Pre my change we would not find a listener, then magically succeed in handling
the load when it was punted back to the docshell.  Which is _very_ wrong -- the
docshell may say it doesn't want a load even if it _could_ handle it -- that's
what the IsPreferred setup is for.

The only reason that loading via a non-click works is that in that case the
docshell does not go up to its parent listener to find out whether to load the
content -- it just looks for a content viewer directly (see CanHandleContent).

The problem I see is that the parent has no way to tell its child "oh, just
decide on your own without me" at the moment.  And this parent listener needs to
exist in order to handle the OnStartURIOpen notification.

I'm considering adding a success nsresult value that the parent listener could
return to indicate to the child listener to just decide on its own.  Then
EmbedContentListener should just use that instead of what it does now....

Thoughts?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Problem is, of course, that nsIURIContentListener is in fact frozen.  I'm not
sure whether the change I propose constitutes an API change or not... (I sorta
think it does, actaully).  And it's not clear to me where I could put the
nsresult value so embedders could get at it -- there seems to be no
error-definition header in docshell/...

I suppose I could just put the old behavior back in place, but it's pretty
broken, imo....

Comment 7

[Boris Zbarsky [:bzbarsky]]

Mental notes:

embedding/browser/photon/src/EmbedContentListener.cpp copied this code and needs
the same fix we do for gtk.

xpfe/components/xremote/src/XRemoteContentListener.cpp just seems confused
(returning NS_OK without setting the out param is just WRONG).

xpfe/browser/resources/content/nsBrowserContentListener.js should be forwarding
isPreferred to the docshell, imo.

embedding/browser/activex/src/control/WebBrowserContainer.cpp does the same
check for a content viewer that the docshell itself does; perhaps the other
embedding listeners should do the same?

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

can you just replace that list with a query to the category manager (the
Gecko-Content-Viewers category)? that list should include all types that mozilla
can render (including plugins).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment 7, last paragraph.. ;)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Seems like the sanest way to go

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 121293 [details] [diff] [review]
Seems like the sanest way to go

Reviews?

I'll not be around tonight, so if this gets reviews, could someone check it in?

Comment 12

[Darin Fisher]

Comment on attachment 121293 [details] [diff] [review]
Seems like the sanest way to go

>Index: embedding/browser/photon/src/EmbedContentListener.cpp

>+  return CanHandleContent(aContentType, PR_TRUE, aDesiredContentType,
>+			  aCanHandleContent);

tabs ok?


> EmbedContentListener::CanHandleContent(const char        *aContentType,
...
> {
>+  if (aContentType) {
...
>+    if (NS_FAILED(rv) && rv != NS_ERROR_NOT_AVAILABLE)
>+      return rv;
>+
>+    if (value && *value)
>+      *_retval = PR_TRUE;
>+  }
>+  return NS_OK;

hmm... if rv == NS_ERROR_NOT_AVAILABLE wouldn't you leave _retval
unassigned?  that seems bad.

maybe:

   *_retval = (value && *value);

then you are ok.  also what does it mean for aContentType == nsnull?
does returning NS_OK without assigning _retval make sense?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

> tabs ok?

The whole file is tab-indented.

> wouldn't you leave _retval unassigned?

Fixed.

> also what does it mean for aContentType == nsnull?

Most likely, that the caller fucked up.  If this were not the day before
freeze, I'd take that test out and replace it with an assert....  For now,
we'll just treat it as a "can't handle".

Comment 14

[Darin Fisher]

Comment on attachment 121303 [details] [diff] [review]
Updated to comments

looks good to me sr=darin

Comment 15

[kurros (Wade Majors)]

Fixed on Trunk

Comment 16

[Boris Zbarsky [:bzbarsky]]

Excuse me?  When did this get r=?

Comment 17

[Christopher Blizzard (:blizzard)]

Comment on attachment 121303 [details] [diff] [review]
Updated to comments

r=blizzard

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 121293 [details] [diff] [review]
Seems like the sanest way to go

(clearing review request on obsolete attachment)