Tom S [:evilpie] Assignee
	Description • 11 years ago

We are using the wrong window to get the documentURI from, DOMWindow can be different from the content.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 1 • 11 years ago

This doesn't seem right to me. We want to be sending the information for DOMWindow, even if it's different from content. In that case, the toplevel property should be false.

	Tom S [:evilpie] Assignee
	Comment 2 • 11 years ago

No this is right, just a bit confusing. In RemoteWebNavigation we do the following: 

this._browser._documentURI = newURI(aMessage.json.documentURI);.

And browser always handles the top level window. I guess it would clearer if we actually just updated our browser information if the top DOMWindow changed.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 3 • 11 years ago

Oh, I see. In that case I think it would be clearer if RemoteWebNavigation does this:

if (aMessage.json.isTopLevel)
  this._browser._documentURI = newURI(aMessage.json.documentURI);

Also, perhaps browser-child.js should only send the documentURI property if isTopLevel is true.

	:Felipe Gomes (needinfo for replies!)
	Comment 4 • 11 years ago

This is the same gotcha as the isTopLevel thing.. See the comment for the isTopLevel getter.

The problem is that in normal occasions there's one webProgress per frame, where in our e10s setup there's only one for the toplevel window.

Still, I think the right thing to do is to call the OnLocationChange callback with the temporary URL for the currently running notification, and reset it to the top-level URL after all OnLocationChange callers have been called.

(Or bite the bullet and create the proper one-webProgress-per-window, since we already have the CPOWs for the windows anyway, from bug 868859)

	:Felipe Gomes (needinfo for replies!)
	Comment 5 • 11 years ago

Comment on attachment 797412 [details] [diff] [review]
document-uri

on irc I believe we agreed that the best to do is to not update the browser+RemoteWebProgress data for a non-toplevel notification

	Tom S [:evilpie] Assignee
	Comment 6 • 11 years ago

I have plans to fix all the problems with RemoteWebProgress with a bigger refactoring, where one browser would have one top level RemoteWebProgress. If we get a non toplevel web-progress event we would dynamically create an object for that.

	Tom S [:evilpie] Assignee
	Comment 7 • 11 years ago

qref fail, sorry!

	:Felipe Gomes (needinfo for replies!)
	Comment 8 • 11 years ago

Should we also use this bug to do the same thing to OnSecurityChange? (I haven't verified yet if it needs the same treatment, but I'm guessing it does)

	Tom S [:evilpie] Assignee
	Comment 9 • 11 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/e7bd5af466ab

	Ed Morley [:emorley]
	Comment 10 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/e7bd5af466ab