333035 - [1.0.8] Context menu broken on form elements (FF, TB, Suite)

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

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

Steps to reproduce:
1) Go to http://google.com/firefox
2a) Right click the text field
2b) Right click the "Google Search" button

Expected results:
a) Text field input context menu (Copy, Select All, Add keyword for this search, etc)
b) Normal context menu

Actual results:
a & b: Image context menu

Seems as though (HTMLInputElement instanceof Components.interfaces.nsIImageLoadingContent) is somehow true at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.296.2.3.2.134.2.16&mark=3796#3794 .

Comment 1

[Carsten Book [:Tomcat]]

confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.13) Gecko/20060404 Firefox/1.0.8

Comment 2

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

Attachment: patch

regression from the branch fix for 301329.
checking for a non-null URI is important, since form elements are instanceof nsIImageLoadingContent but with a null URI.

Comment 3

[Daniel Veditz [:dveditz]]

I broke mail and xpfe the same way:
  mail/base/content/nsContextMenu.js
  xpfe/communicator/resources/content/nsContextMenu.js

Switching product to Core because of missing flag lameness

Comment 4

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

Attachment: mail & xpfe

Comment 5

[Daniel Veditz [:dveditz]]

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

r=dveditz

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 217468 [details] [diff] [review]
mail & xpfe

r=dveditz

Comment 7

[Daniel Veditz [:dveditz]]

Mail isn't so bad, maybe we don't need to re-spin Thunderbird 1.0.8

1) forms in mail aren't all that common (Spam, HTML newsletters)
2) the wrong context menu reads
      Select All    (works correctly)
      Copy          (works correctly)
      Copy Image Location (appears to do nothing)
   In a working build you get the full list of mail actions
   as in the body context menu, plus a "Copy" item.

So you miss a lot of options, but an easy workaround is to move the mouse out of the field and do it there. The essential "Form" options are there and work.

Comment 8

[J. Paul Reed [:preed]]

(In reply to comment #7)
> Mail isn't so bad, maybe we don't need to re-spin Thunderbird 1.0.8

We need to respin Tbird 1.0.8 anyway.

Correct me if I'm wrong, but this will require a 1.7.13 respin too?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

It looks like bits were taken from the first patch on bug 301329 instead of the second.  Getting this change in seems fine, but I really don'

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

It looks like bits were taken from the first patch on bug 301329 instead of the second.  Getting this change in seems fine, but I really don't see why that bug was considered critical for the not-current-branch security release.  Should backing it out be considered instead?  Or at the very least re-verifying the merge was what was landed on the trunk?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, I see, it's a regression from a regression from a patch we want.

Comment 12

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

For what it's worth, I did verify that the other parts of the patch for bug 301329 were correctly merged by comparing the trunk and branch checkins.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, seems ok.

Comment 14

[Daniel Veditz [:dveditz]]

> It looks like bits were taken from the first patch on bug 301329 instead of
> the second.

Not directly. I missed or misunderstood bug 301329 comment 13 and assumed the "this.target.currentURI" check was to protect against the this.target.currentURI.spec" dereference. The old branch didn't have that so I must have assumed the null-check was superfluous. I would not have guessed that a form item was a nsIImageLoadingContent -- that's just wrong.

> Getting this change in seems fine, but I really don't see why that bug
> was considered critical for the not-current-branch security release.

Bug 293527 was made a 1.0.6 blocker during 1.0.5, then auto-bumped by the 1.0.5->1.0.6 respin and 1.0.7 public exploit response releases.

> Should backing it out be considered instead?  Or at the very least
> re-verifying the merge was what was landed on the trunk?

Fixing this bug is a much smaller change than a backout. Comparing the checkins would be worthwhile.

Comment 15

[Tim Riley [:timr]]

This is a critical regression that must be fixed.  a=timr for drivers  Let's go with re-spinning FFx 1.0.8 and Suite 1.7.13.  Skip Tbird 1.0.8 unless mscott wades in.

Comment 16

[Tim Riley [:timr]]

mail & xpfe patch is for the Suite 1.7.13 and for TBird 1.0.8, right?  I'll approve both patches.  But not planning on respinning TBird unless someone screams.

Comment 17

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

(In reply to comment #14)
> I would not have guessed that
> a form item was a nsIImageLoadingContent -- that's just wrong.

It's not wrong. Consider an <input type="image">. And remember that an <input> can change its type to image any time. So, this really must implement this interface at all times if it wants to implement it at any time.

Comment 18

[Tim Riley [:timr]]

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

Critical regression a=timr for drivers.

Comment 19

[Tim Riley [:timr]]

Comment on attachment 217468 [details] [diff] [review]
mail & xpfe

Critical regression. a=timr for drivers.

Comment 20

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

Checked in both patches on the AVIARY_1_0_1_20050124_BRANCH, and the xpfe patch only on the MOZILLA_1_7_BRANCH.

Comment 21

[Boris Zbarsky [:bzbarsky]]

r=me too, fwiw.

Comment 22

[Tracy Walker [:tracy]]

Verified on FFx 1.0.8 Windows respin 20060407

However the Image context menu just regressed to Normal Menu.  see bug 333131

Comment 23

[Jay Patel [:jay]]

FWIW: I verified this again with the latest respins on Windows and Linux, and from Schrep's testing on Mac, things look ok here.