Closed Bug 981900 Opened 6 years ago Closed 6 years ago

nsISHistoryListener does not provide a way to detect history.replaceState()

Categories

(Core :: Document Navigation, defect)

defect
Not set

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.
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.
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.
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 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-
Whiteboard: p=8 s=it-31c-30a-29b.1
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 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)
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)
Whiteboard: p=8 s=it-31c-30a-29b.1 [qa-] → p=8 s=it-31c-30a-29b.2 [qa-]
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Review ping? This missing notification regresses sessionstore and we would really like to uplift the patch if possible.
Hardware: x86 → All
There is no patch waiting for review here. feedback only.
Attachment #8398693 - Flags: review?(bugs)
(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 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(&currentID);
>+
>+      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+
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)
Whiteboard: p=8 s=it-31c-30a-29b.2 [qa-] → p=8 s=it-31c-30a-29b.3 [qa-]
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-
Fixed GetIndexOfEntry() to also check the first entry.
Attachment #8405011 - Attachment is obsolete: true
Attachment #8406573 - Flags: review?(bugs)
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+
Blocks: 1001120
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/a38d0ab6d6b8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Status: RESOLVED → VERIFIED
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.
Hmm, the patch wasn't compiled on Windows.
Duplicate of this bug: 993382
Phil, I am not sure why you asked for tracking here? AFAIK, the patches landed in aurora?
Flags: needinfo?(philringnalda)
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.
Flags: needinfo?(philringnalda)
You need to log in before you can comment on or make changes to this bug.