Closed
Bug 981900
Opened 10 years ago
Closed 10 years ago
nsISHistoryListener does not provide a way to detect history.replaceState()
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
VERIFIED
FIXED
mozilla31
People
(Reporter: smacleod, Assigned: smacleod)
References
Details
(Keywords: dev-doc-needed, Whiteboard: p=8 s=it-31c-30a-29b.3 [qa-])
Attachments
(1 file, 3 obsolete files)
Currently nsISHistoryListener cannot be used to detect a |window.history.replaceState()| call. This is problematic for Bug 967028 as we need to invalidate the session history state for Session Restore when replaceState is called. We should add something like "OnHistoryEntryReplaced" or "OnHistoryEntryModified" to the listener.
Comment 1•10 years ago
|
||
How about OnHistoryReplaceEntry()? It sounds a little awkward but feels more in line with what we currently have. I don't feel very strongly though.
Comment 2•10 years ago
|
||
Just FTR, we discovered that in the case of about:newtab, we need to also dispatch OnHistoryReplaceEntry() in ::AddEntry() as it bails out early if the previous state wasn't persisted (as is the case for about:newtab) and will just override the current history entry i.e. replace it.
Assignee | ||
Comment 3•10 years ago
|
||
This is a first run at the patch which works with Session Store's use case. We probably want better arguments to "OnHistoryReplaceEntry", maybe including the index that's being replaced? I have it just passing the new URI that's replacing the old because Session Store doesn't care right now and I wanted to work out the general approach before worrying about this. Hopefully this patch is sane for the most part :D
Attachment #8392624 -
Flags: feedback?(ttaubert)
Attachment #8392624 -
Flags: feedback?(bugs)
Comment 4•10 years ago
|
||
Comment on attachment 8392624 [details] [diff] [review] Patch - Add OnHistoryReplaceEntry to nsISHistoryListener to handle history.replaceState If I read this correctly, in case an iframe is using replaceState, you end up replacing the SHEntry for the top most document, not for the iframe. (You can check how AddToSessionHistory() ends up dealing that case for !aReplace) I guess one small hack would be to take the current SHEntry from nsISHistory and replace it with itself, or more correctly go through from mOSHE to the top level SHEntry, find its index in SHistory and replace that with itself.
Attachment #8392624 -
Flags: feedback?(bugs) → feedback-
Updated•10 years ago
|
Whiteboard: p=8 s=it-31c-30a-29b.1
Updated•10 years ago
|
Blocks: fxdesktopbacklog
I don't think this needs any special testing for release. Please needinfo me if you think otherwise.
Whiteboard: p=8 s=it-31c-30a-29b.1 → p=8 s=it-31c-30a-29b.1 [qa-]
Comment 6•10 years ago
|
||
Comment on attachment 8392624 [details] [diff] [review] Patch - Add OnHistoryReplaceEntry to nsISHistoryListener to handle history.replaceState Review of attachment 8392624 [details] [diff] [review]: ----------------------------------------------------------------- So... why are we touching the docShell at all here? nsDocShell::AddToSessionHistory() calls nsSHistory::ReplaceEntry() when an entry is replaced. The case mentioned in comment #2 is covered as the caller doesn't request replacing a history entry but because we don't persist the current one it's going to be replaced anyway. That means nsSHistory::AddEntry() is called and all we have to do from there is to call NOTIFY_LISTENERS(OnHistoryReplaceEntry, (uri)) if (!currentPersist). Or am I missing something?
Attachment #8392624 -
Flags: feedback?(ttaubert)
Assignee | ||
Comment 7•10 years ago
|
||
I've updated the patch to properly deal with the case of iframes calling replaceState. Also, I now pass the index of the top level SHEntry to OnHistoryReplaceEntry(...). This makes more sense than passing a url with no reference to what it's replacing. If there is a way to tell the listener the particular entry being replaced, not just the top level entry, that might be better. This definitely covers the Session Restore case though and I'm happy just passing the top level index. (In reply to Tim Taubert [:ttaubert] from comment #6) > So... why are we touching the docShell at all here? > nsDocShell::AddToSessionHistory() calls nsSHistory::ReplaceEntry() when an > entry is replaced. The case mentioned in comment #2 is covered as the caller > doesn't request replacing a history entry but because we don't persist the > current one it's going to be replaced anyway. That means > nsSHistory::AddEntry() is called and all we have to do from there is to call > NOTIFY_LISTENERS(OnHistoryReplaceEntry, (uri)) if (!currentPersist). Or am I > missing something? This is true for the case of non-persistent history entries, but calls to history.replaceEntry take a different code path through docShell.
Attachment #8392624 -
Attachment is obsolete: true
Attachment #8398693 -
Flags: feedback?(bugs)
Assignee | ||
Comment 8•10 years ago
|
||
Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=713c25cb67c1
Updated•10 years ago
|
Whiteboard: p=8 s=it-31c-30a-29b.1 [qa-] → p=8 s=it-31c-30a-29b.2 [qa-]
Updated•10 years ago
|
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Comment 9•10 years ago
|
||
Review ping? This missing notification regresses sessionstore and we would really like to uplift the patch if possible.
Hardware: x86 → All
Comment 10•10 years ago
|
||
There is no patch waiting for review here. feedback only.
Updated•10 years ago
|
Attachment #8398693 -
Flags: review?(bugs)
Comment 11•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #10) > There is no patch waiting for review here. feedback only. I guess I didn't realize that other people may prioritize this very differently from a review request. Sorry!
Comment 12•10 years ago
|
||
Comment on attachment 8398693 [details] [diff] [review] Patch - Add OnHistoryReplaceEntry to nsISHistoryListener to handle history.replaceState v2 Coding style nits, in Gecko {} always with if, and * goes with the type. >+NS_IMETHODIMP >+nsSHistory::GetIndexOfEntry(nsISHEntry * aSHEntry, int32_t * aResult) { >+ nsresult rv; >+ NS_ENSURE_ARG(aSHEntry); >+ NS_ENSURE_ARG_POINTER(aResult); >+ >+ if (mLength <= 0) >+ return NS_ERROR_FAILURE; >+ >+ nsCOMPtr<nsISHTransaction> currentTxn; >+ int32_t cnt = 0; >+ uint32_t entryID; >+ aSHEntry->GetID(&entryID); >+ >+ rv = GetRootTransaction(getter_AddRefs(currentTxn)); >+ if (NS_FAILED(rv) || !currentTxn) >+ return NS_ERROR_FAILURE; >+ >+ while (1) { >+ nsCOMPtr<nsISHTransaction> nextTxn; >+ rv = currentTxn->GetNext(getter_AddRefs(nextTxn)); >+ if (NS_SUCCEEDED(rv) && nextTxn) { >+ cnt++; >+ >+ nsCOMPtr<nsISHEntry> entry; >+ rv = nextTxn->GetSHEntry(getter_AddRefs(entry)); >+ if (NS_FAILED(rv) || !entry) >+ return NS_ERROR_FAILURE; >+ >+ uint32_t currentID; >+ entry->GetID(¤tID); >+ >+ if (entryID == currentID) { >+ *aResult = cnt; >+ break; >+ } else { >+ currentTxn = nextTxn; >+ continue; >+ } I don't understand what this code is doing. Shouldn't you just compare aSHEntry to entry?
Attachment #8398693 -
Flags: review?(bugs)
Attachment #8398693 -
Flags: feedback?(bugs)
Attachment #8398693 -
Flags: feedback+
Assignee | ||
Comment 13•10 years ago
|
||
Switch patch to now compare entry pointers instead of IDs, updated to suggested coding style, and finished off TODO in WebBrowserChrome.cpp
Attachment #8398693 -
Attachment is obsolete: true
Attachment #8405011 -
Flags: review?(bugs)
Updated•10 years ago
|
Whiteboard: p=8 s=it-31c-30a-29b.2 [qa-] → p=8 s=it-31c-30a-29b.3 [qa-]
Comment 14•10 years ago
|
||
Comment on attachment 8405011 [details] [diff] [review] Patch - Add OnHistoryReplaceEntry to nsISHistoryListener to handle history.replaceState v3 >+nsSHistory::GetIndexOfEntry(nsISHEntry* aSHEntry, int32_t* aResult) { >+ nsresult rv; >+ NS_ENSURE_ARG(aSHEntry); >+ NS_ENSURE_ARG_POINTER(aResult); >+ >+ if (mLength <= 0) { >+ return NS_ERROR_FAILURE; >+ } >+ >+ nsCOMPtr<nsISHTransaction> currentTxn; >+ int32_t cnt = 0; >+ >+ rv = GetRootTransaction(getter_AddRefs(currentTxn)); >+ if (NS_FAILED(rv) || !currentTxn) { >+ return NS_ERROR_FAILURE; >+ } >+ >+ while (1) { true >+ nsCOMPtr<nsISHTransaction> nextTxn; >+ rv = currentTxn->GetNext(getter_AddRefs(nextTxn)); >+ if (NS_SUCCEEDED(rv) && nextTxn) { >+ cnt++; >+ >+ nsCOMPtr<nsISHEntry> entry; >+ rv = nextTxn->GetSHEntry(getter_AddRefs(entry)); >+ if (NS_FAILED(rv) || !entry) { >+ return NS_ERROR_FAILURE; >+ } >+ >+ if (aSHEntry == entry) { >+ *aResult = cnt; >+ break; >+ } else { >+ currentTxn = nextTxn; >+ continue; >+ } >+ } else { >+ return NS_ERROR_FAILURE; >+ } >+ } This loop misses index 0, no? You always from the root to at least to next transaction.
Attachment #8405011 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 15•10 years ago
|
||
Fixed GetIndexOfEntry() to also check the first entry.
Attachment #8405011 -
Attachment is obsolete: true
Attachment #8406573 -
Flags: review?(bugs)
Comment 16•10 years ago
|
||
Comment on attachment 8406573 [details] [diff] [review] Patch - Add OnHistoryReplaceEntry to nsISHistoryListener to handle history.replaceState v4 >+nsSHistory::GetIndexOfEntry(nsISHEntry* aSHEntry, int32_t* aResult) { >+ nsresult rv; declare when you first time use this. Could you initialize *aResult = -1; >+ NS_ENSURE_ARG(aSHEntry); >+ NS_ENSURE_ARG_POINTER(aResult);
Attachment #8406573 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a38d0ab6d6b8
Assignee | ||
Updated•10 years ago
|
Keywords: dev-doc-needed
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a38d0ab6d6b8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Comment 19•10 years ago
|
||
Comment on attachment 8406573 [details] [diff] [review] Patch - Add OnHistoryReplaceEntry to nsISHistoryListener to handle history.replaceState v4 >+ AppentIntToCString(info1, status); Appent? You're burning the Windows XULRunner build with that, which either needs to be fixed before the merge to aurora in a few hours, or needs to be fixed on aurora.
Updated•10 years ago
|
tracking-firefox31:
--- → ?
Comment 20•10 years ago
|
||
Hmm, the patch wasn't compiled on Windows.
Assignee | ||
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cfde3603b020
Comment 23•10 years ago
|
||
Phil, I am not sure why you asked for tracking here? AFAIK, the patches landed in aurora?
Flags: needinfo?(philringnalda)
Comment 24•10 years ago
|
||
At the time I requested it, the patch from comment 21 had not yet landed, and it should have been merge-blocking+ between midnight Sunday when I requested it and 9 Monday when smacleod fixed it, because otherwise we would have merged and burned a hidden but nevertheless release-deliverable build.
tracking-firefox31:
? → ---
Flags: needinfo?(philringnalda)
You need to log in
before you can comment on or make changes to this bug.
Description
•