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

RESOLVED FIXED

Status

()

defect
--
major
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

({fixed1.7.13, regression})

1.7 Branch
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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 .
Flags: blocking-aviary1.0.8?
confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.13) Gecko/20060404 Firefox/1.0.8
Keywords: regression
Posted patch patchSplinter Review
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.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #217466 - Flags: review?(mconnor)
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
Component: General → XP Toolkit/Widgets: Menus
Flags: review?(mconnor)
Product: Firefox → Core
Version: 1.0 Branch → 1.7 Branch
Flags: blocking1.7.13?
Summary: [1.0.8] Context menu broken on form elements → [1.0.8] Context menu broken on form elements (FF, TB, Suite)
Attachment #217466 - Flags: superreview?(bzbarsky)
Attachment #217466 - Flags: review?(dveditz)
Attachment #217468 - Flags: superreview?(bzbarsky)
Attachment #217468 - Flags: review?(dveditz)
Comment on attachment 217466 [details] [diff] [review]
patch

r=dveditz
Attachment #217466 - Flags: review?(dveditz) → review+
Comment on attachment 217468 [details] [diff] [review]
mail & xpfe

r=dveditz
Attachment #217468 - Flags: review?(dveditz) → review+
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.
(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 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'
Attachment #217466 - Flags: superreview?(bzbarsky) → superreview+
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?
Attachment #217468 - Flags: superreview?(bzbarsky) → superreview+
Oh, I see, it's a regression from a regression from a patch we want.
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.
Attachment #217466 - Flags: approval-aviary1.0.8?
Attachment #217468 - Flags: approval1.7.13?
Attachment #217468 - Flags: approval-aviary1.0.8?
> 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

13 years ago
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.
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+

Comment 16

13 years ago
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.
(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

13 years ago
Comment on attachment 217466 [details] [diff] [review]
patch

Critical regression a=timr for drivers.
Attachment #217466 - Flags: approval-aviary1.0.8? → approval-aviary1.0.8+

Comment 19

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

Critical regression. a=timr for drivers.
Attachment #217468 - Flags: approval1.7.13?
Attachment #217468 - Flags: approval1.7.13+
Attachment #217468 - Flags: approval-aviary1.0.8?
Attachment #217468 - Flags: approval-aviary1.0.8+
Checked in both patches on the AVIARY_1_0_1_20050124_BRANCH, and the xpfe patch only on the MOZILLA_1_7_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Verified on FFx 1.0.8 Windows respin 20060407

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

Comment 23

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

Updated

11 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: general → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.