Closed Bug 910838 Opened 6 years ago Closed 6 years ago

Don't update RemoteWebProgress info for non-toplevel notifications

Categories

(Core :: Document Navigation, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch document-uri (obsolete) — Splinter Review
We are using the wrong window to get the documentURI from, DOMWindow can be different from the content.
Attachment #797412 - Flags: review?(felipc)
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.
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.
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.
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 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
Attachment #797412 - Flags: review?(felipc)
Attached patch document-uri v2 (obsolete) — Splinter Review
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.
Attachment #797412 - Attachment is obsolete: true
Attachment #798947 - Flags: review?(felipc)
Attached patch document-uri 2Splinter Review
qref fail, sorry!
Attachment #798947 - Attachment is obsolete: true
Attachment #798947 - Flags: review?(felipc)
Attachment #798956 - Flags: review?(felipc)
Attachment #798956 - Flags: review?(felipc) → review+
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)
Summary: Use correct documentURI in RemoteWebProgress → Don't update RemoteWebProgress info for non-toplevel notifications
https://hg.mozilla.org/mozilla-central/rev/e7bd5af466ab
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.