306492 - PFS doesn't take relative urls in pluginspage attribute into consideration

Status: RESOLVED FIXED

(Toolkit Graveyard :: Plugin Finder Service, defect, P1)

Description

[Doron Rosenberg (IBM)]

so if a pluginspage is specified as a relative url ("/bar"), we fail to load the
correct page.

Easy bug, in case someone wants to take it.

Comment 1

[PikeUK]

*** Bug 302995 has been marked as a duplicate of this bug. ***

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This should do it. I tested that it works with frames.

Comment 3

[Doron Rosenberg (IBM)]

Change looks fine.  I guess there is no other way to do this other than using
docshell?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Well, it would be possible to use the
gBrowser.selectedBrowser.webNavigation.currentURI, since this code should
theoretically only run when the corresponding page is displayed, but I figured
since we have a document it'd be safer to get it from that.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Oh, and that also wouldn't work with frames.

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 195598 [details] [diff] [review]
patch

We need to catch the potential exception here if pluginsPage isn't a valid URL.

r=me with that addressed

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: handle invalid pluginsPage

Comment 8

[Doron Rosenberg (IBM)]

do you need this checked in or can you?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> do you need this checked in or can you?

Hrm, I thought I had checked it in. I can check it in later today, but if you
want to go ahead and do it now that's fine by me.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Trunk:
mozilla/browser/base/content/browser.js; new revision: 1.504;

Comment 11

[Mike Schroepfer]

Comment on attachment 196147 [details] [diff] [review]
handle invalid pluginsPage

Approved for 1.8b5 per bug meeting

Comment 12

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/base/content/browser.js; new revision: 1.479.2.30;

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Damnit, I forgot to land the contentAreaUtils.js changes from the original
patch. Will land that later today.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Landed the other part of the patch:
Trunk: mozilla/toolkit/content/contentAreaUtils.js; new revision: 1.79;
Branch: mozilla/toolkit/content/contentAreaUtils.js; new revision: 1.77.2.2;

Comment 15

[Doron Rosenberg (IBM)]

Ugh, the patch is actually bad.  It will create a url for an "" pluginspage (aka no pluginspage defined), so we show the manual url button always (which will then load the site you were at).

Comment 16

[Doron Rosenberg (IBM)]

Attachment: fix the error

Only do this if we have a pluginspage

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Attachment: check for empty pluginspage

I suck... not able to test at the moment, feel free to take and run with it.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 203434 [details] [diff] [review]
check for empty pluginspage

hmm, no midair

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Landed that on the trunk. mconnor said it`s probably not 1.8.0.1-worthy, so targetting for 1.8.1, I guess...

Sorry about the regression :(

mozilla/browser/base/content/browser.js; new revision: 1.530;

Comment 20

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

+      var docShell = findChildShell(doc, gBrowser.selectedBrowser.docShell, null);

What's wrong with gBrowser.currentURI?

Comment 21

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

oh, that'd break child frames I guess. nevermind.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 203433 [details] [diff] [review]
fix the error

Requesting approval for this very low risk regression fix.

Comment 23

[Mike Schroepfer]

Comment on attachment 203433 [details] [diff] [review]
fix the error

Please consider for 1.8.1 - 1.8.0.1 is for major security and crash issues only.

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 203433 [details] [diff] [review]
fix the error

resetting approval request, not sure it was clear this was a regression.

Comment 25

[Daniel Veditz [:dveditz]]

(In reply to comment #19)
> Landed that on the trunk. mconnor said it`s probably not 1.8.0.1-worthy, so
> targetting for 1.8.1, I guess...

If this is re-fixed on the trunk please mark the bug resolved-fixed.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Yes, this was re-fixed on the trunk.

A new bug should have been filed on the regression, it would have been easier to track. To be clear, I'll resummarize:

This patch is a simple fix to the original patch that went in for Firefox 1.5. The net effect is that in Firefox 1.5, the Plugin Finder always shows the "Manual" button, and in some cases it will load the same page the user is looking at. The patch essentially reverts to the correct 1.0.x behavior without regressing the original issue fixed in this bug.

Comment 27

[Daniel Veditz [:dveditz]]

approvals not yet necessary for 1.8.1 -- you can land reviewed patches there the same as on the trunk right now.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Fixed on the 1.8 branch.
mozilla/browser/base/content/browser.js; new revision: 1.479.2.49;