If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

bug 795065 broke calling openUILink without an event parameter

RESOLVED FIXED in Firefox 18

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gavin, Assigned: mak)

Tracking

Trunk
Firefox 20
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17 unaffected, firefox18+ fixed, firefox19+ fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

See bug 795065 comment 51.
Blocks: 795065
Created attachment 684258 [details] [diff] [review]
patch
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #684258 - Flags: review?(mak77)

Comment 2

5 years ago
Won't passing null cause an error when it eventually reaches:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/contentAreaUtils.js#339
339     initiatingWindow  : aInitiatingDocument.defaultView
(Assignee)

Comment 3

5 years ago
One side I think it's bad to break compatibility. On the other side, if we keep accepting that, I fear add-ons will never handle privatebrowsing properly.
Do we have an idea of how many add-ons are involved here?

Btw, philip is right that this would fail down in contentAreaUtils, not just there but also internalPersist wants initiatingWindow to be non null and doesn't handle the other case.
(Assignee)

Updated

5 years ago
Attachment #684258 - Flags: review?(mak77) → review-
Summary: bug 795065 broke calling openLinkIn without an event parameter → bug 795065 broke calling openUILink without an event parameter
(In reply to Marco Bonardo [:mak] from comment #3)
> One side I think it's bad to break compatibility. On the other side, if we
> keep accepting that, I fear add-ons will never handle privatebrowsing
> properly.
> Do we have an idea of how many add-ons are involved here?

https://mxr.mozilla.org/addons/search?string=openUILink%28&find=&findi=&filter=openUILink\%28[^%2C]%2B\%29%3B&hitlimit=&tree=addons

shows 364 files affected, and that's a conservative search (I didn't spend much time trying to create a regex that would catch all possible cases).

It's actually impossible for this code to ever reach the saveURL path that requires the document, right? whereToOpenLink won't return "save" if the event object is null. Given that, there should be no concern about people improperly handling PB mode.

Clearly I didn't test the patch, sorry about that. Do one of you/Philip/jdm want to take this bug?
(Assignee)

Comment 5

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> https://mxr.mozilla.org/addons/
> search?string=openUILink%28&find=&findi=&filter=openUILink\%28[^%2C]%2B\%29%3
> B&hitlimit=&tree=addons
> 
> shows 364 files affected, and that's a conservative search (I didn't spend
> much time trying to create a regex that would catch all possible cases).

I see, it's the case where it's better to avoid breaking and do multiple call-to-action blog posts.

> It's actually impossible for this code to ever reach the saveURL path that
> requires the document, right? whereToOpenLink won't return "save" if the
> event object is null. Given that, there should be no concern about people
> improperly handling PB mode.

Right, OpenLinkIn uses aInitiatingDoc only if where == "save" and whereToOpenLink doesn't return "save" without an event.
This means your patch may actually just work, unless someone calls directly into openLinkIn. Though looks like weak protection, we may add some additional check in internalSave and internalPersist.

May be worth adding a reportError to any call to OpenUILink that doesn't provide an event? Something like "An add-on is invoking OpenUILink without an event. This is deprecated cause doesn't allow to properly handle Private Browsing mode."? This may help detecting and fixing callers.
tracking-firefox19: --- → ?
Yes, adding a warning for cases that could be mishandled is a good idea.
tracking-firefox18: --- → +
tracking-firefox19: ? → +
(Assignee)

Comment 7

5 years ago
Philip do you want to take the bug? I may review then.

Comment 8

5 years ago
> Philip do you want to take the bug? I may review then.
I'm sorry but I can't at the moment as real life issues are taking up most of my time.
(Assignee)

Comment 9

5 years ago
Ok, I'm taking this
Assignee: gavin.sharp → mak77
(Assignee)

Comment 10

5 years ago
This is a bit more complex than expected, since

openLinkIn / saveURL / internalSave / internalPersist

contains

let privacyContext = persistArgs.initiatingWindow.QueryInterface(...)...;
persist.saveURI(..., privacyContext);

The nsIWebBrowserPersist idl states:

@param aPrivacyContext A context from which the privacy status of this
*                   save operation can be determined. Must only be null
*                   in situations in which no such context is available
*                   (eg. the operation has no logical association with any
*                   window or document)

So, while passing null here is supported, doing so cause of a missing argument is breaking the interface rules. And relaxing the interface doesn't sound like a good idea for future.

In the end I think it's better to just keep the checks in utilityOverlay, without going down to toolkit.

Just wondering if I should reportError for cases OpenLinkIn is invoked without an event, or just the specific case of OpenLinkIn with where == "save"  but no initiatind document provided, that is the specific case it may happen.
I fear the former would be a bit too much verbose with so many add-ons misbehaving.
(Assignee)

Comment 11

5 years ago
Funny times, we had a bc test for this (browser_utilityOverlay.js) but it was never added to the makefile.
(Assignee)

Comment 12

5 years ago
Created attachment 685253 [details] [diff] [review]
patch v1.1

I have revised the test while there... it is definitely incomplete compared to utilityOverlay, but making a complete test was out of the scope of this bug
Attachment #684258 - Attachment is obsolete: true
Attachment #685253 - Flags: review?(gavin.sharp)
Comment on attachment 685253 [details] [diff] [review]
patch v1.1

>diff --git a/browser/base/content/test/Makefile.in b/browser/base/content/test/Makefile.in

>+                 browser_utilityOverlay.js \

Doh! This one was my fault :)

>diff --git a/browser/base/content/test/browser_utilityOverlay.js b/browser/base/content/test/browser_utilityOverlay.js

>+function runNextTest() {
>+  while (gBrowser.tabs.length > 1) {
>+    gBrowser.removeCurrentTab();

Can you remove this?

> function test_openUILink() {

... and instead have this test clean up its added tab specifically?

I don't like taking the lazy approach to test cleanup, since it makes debugging failures when they occur more mysterious.

>diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js

>   if (where == "save") {
>-    saveURL(url, null, null, true, null, aReferrerURI, aInitiatingDoc);
>+    if (aInitiatingDoc) {
>+      saveURL(url, null, null, true, null, aReferrerURI, aInitiatingDoc);
>+    } else {
>+      Components.utils.reportError(
>+        "OpenLinkIn has been invoked without an initiating document.\n" +
>+        "This may be caused by an add-on miscalling into it, or overriding " +
>+        "some utilityOverlay.js methods.  Please contact the add-on author."
>+      );

nit: I think I'd prefer:
if (where == "save") {
  if (!aInitiatingDoc) {
    Components.utils.reportError("openUILink called with where=='save' without aInitiatingDoc - see https://bugzilla.mozilla.org/show_bug.cgi?id=814264.");
    return;
  }
  saveURL(...);
  return;
}

(bug link instead of the more generic "contact the author", whose identity is probably not obvious at all - people should be able to get the relevant info or contact the right people from this bug)

thanks for taking this!
Attachment #685253 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 685639 [details] [diff] [review]
patch v1.2
Attachment #685253 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fae627fef335
status-firefox17: --- → unaffected
status-firefox18: --- → affected
status-firefox19: --- → affected
Flags: in-testsuite+
Target Milestone: --- → Firefox 20
(Assignee)

Comment 16

5 years ago
Comment on attachment 685639 [details] [diff] [review]
patch v1.2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 795065
User impact if declined: lots of add-ons breakage
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): very low, it's a null check
String or UUID changes made by this patch: none
Attachment #685639 - Flags: approval-mozilla-beta?
Attachment #685639 - Flags: approval-mozilla-aurora?
Backed out for browser_locationBarCommand.js timeouts:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=fae627fef335

https://hg.mozilla.org/integration/mozilla-inbound/rev/00d402add716

Updated

5 years ago
Attachment #685639 - Flags: approval-mozilla-beta?
Attachment #685639 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 18

5 years ago
oops, that browser_locationBarCommand.js test must be updated, it is exactly hitting the dark side of privacy awareness.
(Assignee)

Comment 19

5 years ago
Created attachment 685764 [details] [diff] [review]
patch v1.3

In the end, the reportError allowed me to find that we are not handling properly privacy context in urlbarBindings.xml::handleCommand.
In addition I did a search of any 'LinkIn(' call in mxr and triaged all of them manually to check there were not more instances of the problem.

The urlbarBindings.xml is the only interesting change in need of review, compared to previous version.

Pushing this to try, just in case, but works fine locally.
Attachment #685639 - Attachment is obsolete: true
Attachment #685764 - Flags: review?(gavin.sharp)
(Assignee)

Comment 20

5 years ago
Created attachment 685766 [details] [diff] [review]
patch v1.3 (the real one)

the right patch?
Attachment #685764 - Attachment is obsolete: true
Attachment #685764 - Flags: review?(gavin.sharp)
Attachment #685766 - Flags: review?(gavin.sharp)
Comment on attachment 685766 [details] [diff] [review]
patch v1.3 (the real one)

r=me with the urlbarBindings caller changed to just use "document" all the time, per discussion on IRC.
Attachment #685766 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 22

5 years ago
Created attachment 685771 [details] [diff] [review]
patch v1.4

Thanks, pushing this to try.
Attachment #685766 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
looks green
https://tbpl.mozilla.org/?tree=Try&rev=ecef54363440
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb4eff0fc3d7
(Assignee)

Comment 25

5 years ago
Comment on attachment 685771 [details] [diff] [review]
patch v1.4

OK, now it's sticking in the tree.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 795065
User impact if declined: lots of add-ons breakage
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): very low, it's a null check
String or UUID changes made by this patch: none
Attachment #685771 - Flags: approval-mozilla-beta?
Attachment #685771 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 26

5 years ago
Comment on attachment 685771 [details] [diff] [review]
patch v1.4

I misread the nit in comment 13 and didn't notice it was also suggesting using a different error text, so I updated the text here

https://hg.mozilla.org/integration/mozilla-inbound/rev/bae3069ca396

Now merging patches and re-asking approval on the merge.
Attachment #685771 - Attachment is obsolete: true
Attachment #685771 - Flags: approval-mozilla-beta?
Attachment #685771 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #685771 - Attachment is obsolete: false
(Assignee)

Comment 27

5 years ago
Created attachment 686151 [details] [diff] [review]
coalesced patch for Aurora/Beta

[Approval Request Comment]
See comment 25
Attachment #686151 - Flags: approval-mozilla-beta?
Attachment #686151 - Flags: approval-mozilla-aurora?
Comment on attachment 686151 [details] [diff] [review]
coalesced patch for Aurora/Beta

Low risk null check for a FF18 regression - approving for branches.
Attachment #686151 - Flags: approval-mozilla-beta?
Attachment #686151 - Flags: approval-mozilla-beta+
Attachment #686151 - Flags: approval-mozilla-aurora?
Attachment #686151 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/eb4eff0fc3d7
https://hg.mozilla.org/mozilla-central/rev/bae3069ca396
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 30

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/935abd776974
https://hg.mozilla.org/releases/mozilla-beta/rev/714c52485f59
status-firefox18: affected → fixed
status-firefox19: affected → fixed

Updated

5 years ago
Blocks: 821328
You need to log in before you can comment on or make changes to this bug.