Closed Bug 1220160 Opened 4 years ago Closed 4 years ago

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


(Firefox :: Menus, defect)

Not set



Firefox 47
Tracking Status
firefox45 --- affected
firefox47 --- fixed


(Reporter: Gijs, Assigned: Gijs)




(3 files)

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?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(MattN+bmo)
Or better yet we could have a chromeonly getter on document that does the right thing, even in the neterror/certerror case?
Flags: needinfo?(bzbarsky)
(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.
Flags: needinfo?(MattN+bmo) → needinfo?(gijskruitbosch+bugs)
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. :-\
Attachment #8719615 - Flags: review?(bzbarsky)
Assignee: nobody → gijskruitbosch+bugs
(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. :-\
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8719615 [details] [diff] [review]
part 1: add chromeonly getter for documenturi that returns the original document when on an error page,


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!
Attachment #8719615 - Flags: review?(bzbarsky) → review+
Keywords: leave-open
Attachment #8722137 - Flags: review?(past) → review+
Comment on attachment 8722137 [details]
MozReview Request: Bug 1220160 - part 3: use mozDocumentURIIfNotForErrorPages for ssl error reporting, r?past

Nice simplification!
Attachment #8722136 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8722136 [details]
MozReview Request: Bug 1220160 - part 2: use mozDocumentURIIfNotForErrorPages for context menu's docLocation, r?MattN
Keywords: leave-open
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.