Closed Bug 1173215 Opened 9 years ago Closed 9 years ago

framescripts have incorrect documentURI after history.pushState

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: mixedpuppy, Assigned: smaug)

References

Details

Attachments

(2 files)

Attached patch pushstateSplinter Review
Attached test diff will fail on the second "is" test.  documentURI in the framescript remains the first loaded uri.
This works for me at first glance if I just do this in the console on a web page:

  history.pushState({}, "", "test.html"); document.documentURI

so what's weird about the frame script case?
(In reply to Not doing reviews right now from comment #1)
> This works for me at first glance if I just do this in the console on a web
> page:
> 
>   history.pushState({}, "", "test.html"); document.documentURI
> 
> so what's weird about the frame script case?

After the pushState browser console will show gBrowser.currentURI correct, and the web console will show the correct documentURI.  However, code running inside content.js does not have the correct documentURI (content.document.documentURI).
Is content.js using a CPOW or something for the document here, I wonder?
moving conversation here...

last comment from smaug:

(gBrowser.currentURI refers to the URI docshell knows about and content.document.documentURI is about, well, the document URI.)

Anyhow, what is content.document.documentURI right after content.history.pushState({}, "2", "2.html"); ?

Does the testcase lead us to start loading "http://example.com", but before the load is ready,
pushState() is called, and then the page loading finishes, and document.documentURI is set back to
"http://example.com"

(I don't know how BrowserTestUtils.openNewForegroundTab behaves.)
@smaug:

openNewForegroundTab waits for load so the test is not failing due to that.  

I changed the test locally, doesn't matter whether I wait or return immediately after the pushState call, content.document.documentURI is incorrect.

Loading a web page with the following code works as expected showing code inside the web page has the correct url.  (note this cannot run as a file: url)

window.onload = function() {
    alert(document.documentURI); // original url
    history.pushState({}, "2", "2.html");
    alert(document.documentURI); // new url from pushState
}


so...

- chrome access (eg. gBrowser.currentURI via browser console) is correct
- document.documentURI inside the web page is correct
- document.documentURI inside the web console is correct
- content.document.documentURI inside a framescript is wrong, not updated after pushState
ni for comment 5.
Flags: needinfo?(bugs)
This is really odd.
content.document.documentURI doesn't even end up calling nsIDocument::GetDocumentURI, but
content.eval('document.documentURI'); does.
Where do we get the initial content.document.documentURI value and where is it cached?
Flags: needinfo?(bugs)
Here's a better question.  What is content.document in this case?  e.g. if you breakpoint in js::math_sin, then do Math.sin(content.document) and then examine what first-arg value, what is the JSClass?
I'm looking into this. Will report back.
Assignee: nobody → mconley
My endeavors led me into XrayWrapper land, where I quickly got lost.

I handed some STR over to bz, who said he'd dig in.
Flags: needinfo?(bzbarsky)
OK, this is completely ridiculous.

Bindings.conf has this bit for Document:

    'binaryNames': {
        'documentURI': 'documentURIFromJS',

And documentURIFromJS will do GetDocumentURI if !mChromeXHRDocURI and caller is not chrome, else will return mChromeXHRDocURI.

Now the problem is, we set mChromeXHRDocURI in ResetToURI.  Similarly, we set mChromeXHRDocBaseURI in nsDocument::Reset.  Olli, why are we doing that?  It seems pretty weird to me, and in particular it seems like we should _only_ do that if we already have a non-null mChromeXHRDocURI/mChromeXHRDocBaseURI.
Flags: needinfo?(bzbarsky) → needinfo?(bugs)
Reading through bug 859095
Depends on: 859095
And the answer is, Olli has done a bad review.
Flags: needinfo?(bugs)
Assignee: mconley → bugs
Attached patch v1Splinter Review
Just reset to use the normal URIs if we would have set
mChrome* uris to those values anyway.
Attachment #8621380 - Flags: review?(bzbarsky)
Comment on attachment 8621380 [details] [diff] [review]
v1

I'm not sure I follow.  Doesn't the Reset/ResetToURI happen after XHR does the sets of these members on the document?  So just setting them to null there seems like it would clobber the values XHR set...
When XHR creates a document it sets document uri based on the channel's uri, but 
chromeXHRDocURI points to owner's document uri (as XHR document's documentURI used to).

Now we call StartDocumentLoad, usually with reset=false, so we don't end up resetting the
documenturi nor chromeXHRDocURI.

In case we do reset, we anyway set chromeXHRDocURI to the new uri.
Comment on attachment 8621380 [details] [diff] [review]
v1

> Now we call StartDocumentLoad, usually with reset=false

Ah, that's the part I was missing.

r=me
Attachment #8621380 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/c219548106bb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8621380 [details] [diff] [review]
v1

I'm requesting this since bug 1155014 is blocked on this, would like to get this problem addressed in 40.  Patch seems low risk but perhaps bz can chime in, I'm not familiar with this area of code.

Approval Request Comment
[Feature/regressing bug #]: Share, bug 1155014
[User impact if declined]: sites that use history.pushState will not share the correct url
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: low?
[String/UUID change made/needed]: none
Flags: needinfo?(bzbarsky)
Attachment #8621380 - Flags: approval-mozilla-aurora?
Comment on attachment 8621380 [details] [diff] [review]
v1

This should be rather safe for Aurora.
Flags: needinfo?(bzbarsky)
Depends on: 1174951
Blocks: 1175306
I've a suspicion this is not actually fixed, gBrowser.selectedBrowser.documentURI is incorrect now after pushState.

On a fresh nightly build:

open tab to mozilla.org
open web console
open browser toolbox

In browser toolbox:

gBrowser.currentURI.spec
"https://www.mozilla.org/en-US/"
gBrowser.selectedBrowser.documentURI.spec
"https://www.mozilla.org/en-US/"

In web console:

location
Location → https://www.mozilla.org/en-US
history.pushState(null, "foo", "foo.html");
location
Location → https://www.mozilla.org/en-US/foo.html

In browser toolbox:

gBrowser.currentURI.spec
"https://www.mozilla.org/en-US/foo.html"
gBrowser.selectedBrowser.documentURI.spec
"https://www.mozilla.org/en-US/"


documentURI in a framescript seems to always have the correct url now.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
Does this work with e10s disabled? If so, likely bug 1170488.
Flags: needinfo?(mixedpuppy)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #23)
> Does this work with e10s disabled? If so, likely bug 1170488.

Indeed, it is fine with non-e10s.  Bug 1170488 it is.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
Comment on attachment 8621380 [details] [diff] [review]
v1

Should be safe, taking it.
Attachment #8621380 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.