1220160 - Figure out how context menu data's docLocation should be set

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[:Gijs (he/him)]

Maybe the sanest thing would actually be to always use document.documentURI except if that is about:neterror/about:certerror (and then use document.location.href)?

Matt/Boris, does that sound sensible?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Or better yet we could have a chromeonly getter on document that does the right thing, even in the neterror/certerror case?

Comment 2

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Or better yet we could have a chromeonly getter on document that does the
> right thing, even in the neterror/certerror case?

I always appreciate when people point out I'm limiting the scope of my solutions to a problem to a subset of the code we own, to the detriment of the quality of the solution. :-)

I'll have a look at this sometime this week.

Comment 3

[:Gijs (he/him)]

Attachment: part 1: add chromeonly getter for documenturi that returns the original document when on an error page,

Something like this? I'm not sure whether I need to addref for webidl things - I got crashes when I didn't do this for the result of NS_NewURI in an earlier attempt to implement this. I expect it wouldn't crash here either way (though I've not tried) because I'm essentially returning members that would be kept alive by their owning document/channel anyway, but I figured it would make sense to keep the addrefs - maybe it doesn't and I'm missing something because I'm tired. :-\

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> I'll have a look at this sometime this week.

Clearly not quite this week. But hey, here's a patch for the core bit. If that looks OK I'll write up the nsContextMenu.js. Oh, and I'll take suggestions for the name of the member variable here... mozDocumentURIBeforeErrors doesn't sound great, but I struggled to come up with something better. :-\

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8719615 [details] [diff] [review]
part 1: add chromeonly getter for documenturi that returns the original document when on an error page,

>+nsIURI*

Should be already_AddRefed<nsIURI> for the return value.  Otherwise this leaks.

>+  nsCOMPtr<nsIURI> uri = GetDocumentURIObject();

Can't we just move this to after the mFailedChannel block?  I don't see why we care about it until then.

>+      NS_ADDREF(failedURI);
>+      return failedURI;

Please return those two lines with:

  return failedURI.forget();

which will incidentally fail to compile if you don't change the return type to already_AddRefed<nsIURI>, thus catching the leak for you.  ;)

>+  NS_ADDREF(uri);
>+  return uri;

Again, uri.forget().

>+  [ChromeOnly] readonly attribute URI? mozDocumentURIBeforeErrors;

Naming things is hard.  :(

Maybe mozDocumentURIIfNotForErrorPages or something?  Note that for network error pages it doesn't return the "original URI" in the sense of channel originalURI; it returns the URI of the thing we were trying to load when we got the network error, whatever that is.

r=me with those nits.  Thank you for doing this!

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/164d5bc71555

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/164d5bc71555

Comment 8

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1220160 - part 2: use mozDocumentURIIfNotForErrorPages for context menu's docLocation, r?MattN

Review commit: https://reviewboard.mozilla.org/r/35877/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35877/

Comment 9

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1220160 - part 3: use mozDocumentURIIfNotForErrorPages for ssl error reporting, r?past

Review commit: https://reviewboard.mozilla.org/r/35879/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35879/

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8722137 [details]
MozReview Request: Bug 1220160 - part 3: use mozDocumentURIIfNotForErrorPages for ssl error reporting, r?past

https://reviewboard.mozilla.org/r/35879/#review32709

Nice simplification!

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8722136 [details]
MozReview Request: Bug 1220160 - part 2: use mozDocumentURIIfNotForErrorPages for context menu's docLocation, r?MattN

https://reviewboard.mozilla.org/r/35877/#review32791

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2142620090e7
https://hg.mozilla.org/integration/fx-team/rev/e4be65c87fcc

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1b9d91965921

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2142620090e7
https://hg.mozilla.org/mozilla-central/rev/e4be65c87fcc
https://hg.mozilla.org/mozilla-central/rev/1b9d91965921