910838 - Don't update RemoteWebProgress info for non-toplevel notifications

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Tom S [:evilpie]]

Attachment: document-uri

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

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 2

[Tom S [:evilpie]]

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.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

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)

Comment 5

[:Felipe Gomes (needinfo for replies!)]

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

Comment 6

[Tom S [:evilpie]]

Attachment: document-uri v2

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.

Comment 7

[Tom S [:evilpie]]

Attachment: document-uri 2

qref fail, sorry!

Comment 8

[:Felipe Gomes (needinfo for replies!)]

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)

Comment 9

[Tom S [:evilpie]]

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

Comment 10

[Ed Morley [:emorley]]

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