883968 - Replacing notificationCallbacks prevents plugin content from getting loaded

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Georg Koppen]

Attachment: test.xpi

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/20100101 Firefox/17.0 (Beta/Release)

Steps to reproduce:

I opened twitch.tv having a (test) extension (which is attached) in a clean new profile both in a stable Firefox (21.0) and the recent beta (22.0b5).


Actual results:

The flash content gets loaded in the stable but not the beta and later versions.


Expected results:

The flash content should get loaded in 22.0b5 and later versions as well.

I bisected that problem down to https://hg.mozilla.org/mozilla-central/rev/f22f257f7330

Comment 1

[Benjamin Smedberg]

What does "replacing notificationCallbacks" mean in this context?

Comment 2

[John Schoenick [:johns]]

Attachment: The extension JS in question

Comment 3

[Georg Koppen]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> What does "replacing notificationCallbacks" mean in this context?

Doing things like

this.origCbs = httpChannel.notificationCallbacks;
httpChannel.notificationCallbacks = new NotCallbackTester(this.origCbs);

and NotCallbackTester could then e.g. provide an own implementation of nsIAuthPromptProvider and friends while giving back the original notification callbacks if other interfaces are requested.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Exciting.

So nsObjectLoadingContent::OpenChannel passes "this" as the notification callbacks.  But of course the JS object no longer implements nsIInterfaceRequestor, so the this.originalNotificationCallbacks.getInterface(aIID) thing in the attached extension would throw.  The extension then swallows the exception, which probably didn't make debugging that any more fun.

We fixed this for XMLHttpRequest in bug 780529.  I guess we need something like that here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So I'm going to do a simpler change here: just a shim interface requestor that forwards nsIChannelEventSink to the object loading content.

Comment 6

[Boris Zbarsky [:bzbarsky]]

And we'll hope that no one assumes they can get the node from the channel via the notification callbacks here here...

If people _do_ assume that, we need something more like the XHR fix.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, I have a patch.... but I can't reproduce this bug.  If I install this extension and go to http://www.speakeasy.net/speedtest/ the plug-in is instantiated just fine.

Georg, what's a url that shows the problem?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 9

[Georg Koppen]

http://www.twitch.tv/ (that got buried in my bug description, sorry).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 763754 [details] [diff] [review]
Patch

Thanks, I can reproduce there and the patch fixes it.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: And with a test fix based on try run

Comment 12

[John Schoenick [:johns]]

Comment on attachment 764343 [details] [diff] [review]
And with a test fix based on try run

Review of attachment 764343 [details] [diff] [review]:
-----------------------------------------------------------------

With the caveat that I'm not very familiar with our XPCOM or cycle collector macros, this lgtm

Comment 13

[Andrew McCreight [:mccr8]]

CC stuff looks okay to me.

Comment 14

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/798c709e591f

Unfortunately, this was reported too late to make it into 22, but I think we should backport to 23...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 764343 [details] [diff] [review]
And with a test fix based on try run

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 827158 
User impact if declined: Some extensions will cause plug-ins to not work at all.
Testing completed (on m-c, etc.): Passes the testcase in the bug.
Risk to taking this patch (and alternatives if risky): Risk is medium to low.  The other alternative would be disabling WebIDL bindings for <object>, which is
   probably more risky.
String or IDL/UUID changes made by this patch:  None.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> User impact if declined: Some extensions will cause plug-ins to not work at all.

"In some cases".  Probably involving redirects...

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/798c709e591f

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7462b76587aa

Comment 19

[johnrehta]

Well, I am facing the same issue while visiting the site https://imobdro.com