Closed Bug 1364364 Opened 7 years ago Closed 7 years ago

nsSHEntryShared::RemoveFromBFCacheSync/Async will not remove dynamic entries.

Categories

(Core :: DOM: Navigation, enhancement, P2)

Product:
Core
Component:
DOM: Navigation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: freesamael, Assigned: freesamael)

References

Details

Attachments

(7 files)

Bug 1364364 - Part 1: Why are we declaring private virtual functions?
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1364364 - Part 2: Extract a part of EvictExpiredContentViewerForEntry to a new function FindTransactionForBFCache, and implement RemoveDynEntriesForEntry on top of it.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1364364 - Part 3: Clear dynamic subframe entries in RemoveFromBFCacheSync/Async.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1364364 - Part 4: Add test case.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1364364 - Part 5.1: Rename frame/iframe.html of to-be-modifed test cases.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1364364 - Part 5.2: Remove browser.sessionhistory.cache_subframes and fix test cases relying on it.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1364364 - Part 6: Re-connect nsDocViewerFocusListener in nsDocumentViewer::Open.
59 bytes, text/x-review-board-request
smaug
: review+
Details 
We remove dynamic entries on dropping bfcache, but only if it goes tho nsSHistory::EvictContentViewerForTransaction [1]. nsSHEntryShared::RemoveFromBFCacheSync/Async wouldn't.

[1] http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/docshell/shistory/nsSHistory.cpp#230
Priority: -- → P2
Assignee: nobody → sawang
WIP patches. Still thinking how we can test this.
We've made assumptions almost everywhere that bfcache only work in root entries. Should we remove the 2 lines and related test cases?

http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/docshell/base/nsDocShell.cpp#8303-8304
Yeah, I think we could remove that. browser.sessionhistory.cache_subframes has probably never been true, and I don't see how we could enabled it.
Comment on attachment 8901072 [details]
Bug 1364364 - Part 1: Why are we declaring private virtual functions?

https://reviewboard.mozilla.org/r/172546/#review180422

I guess someone just copy-pasted some other method.
Attachment #8901072 - Flags: review?(bugs) → review+
Comment on attachment 8901073 [details]
Bug 1364364 - Part 2: Extract a part of EvictExpiredContentViewerForEntry to a new function FindTransactionForBFCache, and implement RemoveDynEntriesForEntry on top of it.

https://reviewboard.mozilla.org/r/172548/#review180426
Attachment #8901073 - Flags: review?(bugs) → review+
Comment on attachment 8901074 [details]
Bug 1364364 - Part 3: Clear dynamic subframe entries in RemoveFromBFCacheSync/Async.

https://reviewboard.mozilla.org/r/172550/#review180428
Attachment #8901074 - Flags: review?(bugs) → review+
Comment on attachment 8903456 [details]
Bug 1364364 - Part 4: Add test case.

https://reviewboard.mozilla.org/r/175302/#review180436

aha, interesting stuff. Good to have this kinds of tests!
Attachment #8903456 - Flags: review?(bugs) → review+
Comment on attachment 8904504 [details]
Bug 1364364 - Part 5.2: Remove browser.sessionhistory.cache_subframes and fix test cases relying on it.

https://reviewboard.mozilla.org/r/176356/#review181242

::: dom/workers/test/test_multi_sharedWorker_lifetimes.html:108
(Diff revision 1)
> +            // entryGlobal being the TabChildGlobal (and that would make the
> +            // baseURI in the location assignment below being incorrect);
> +            // setTimeout on the otherhand ensures the entryGlobal is this
> +            // window.
>              info("Waiting the event queue to clear");
> -            SpecialPowers.executeSoon(sendToGenerator);
> +            setTimeout(sendToGenerator, 0);

I'm still confused about the ScriptSettingsStack:

If I keep SpecialPowers.executeSoon at this line, on the location assignment below, ScriptSettingsStack::Top() will be the entry of TabChildGlobal, and the global for this test window is not in the stack at all. Is it an intentional design?
Comment on attachment 8904503 [details]
Bug 1364364 - Part 5.1: Rename frame/iframe.html of to-be-modifed test cases.

https://reviewboard.mozilla.org/r/176354/#review181260

I don't know why this change, but at least shouldn't cause harm :)
Attachment #8904503 - Flags: review?(bugs) → review+
Comment on attachment 8904504 [details]
Bug 1364364 - Part 5.2: Remove browser.sessionhistory.cache_subframes and fix test cases relying on it.

https://reviewboard.mozilla.org/r/176356/#review181266

ah, I see.  Good to get rid of that pref indeed.
Attachment #8904504 - Flags: review?(bugs) → review+
Comment on attachment 8904505 [details]
Bug 1364364 - Part 6: Re-connect nsDocViewerFocusListener in nsDocumentViewer::Open.

https://reviewboard.mozilla.org/r/176358/#review181268

aha. (We really should get rid of nsDocViewerFocusListener. It is really old and weird. Should probably move that event handling to nsFocusManager.)
Attachment #8904505 - Flags: review?(bugs) → review+
Um...The combination of test_multi_sharedWorker_lifetime.html changes and Part 6 fix somehow causes an assertion running test_onLine.html

Assertion failure: IsSafeToRun(), at /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/SchedulerGroup.h:81
https://treeherder.mozilla.org/logviewer.html#?job_id=128492449&repo=try&lineNumber=6588
Oh, it's not about part 6. Just that the change of test_multi_sharedWorker_lifetime.html will make tabbrowser try to add a hidden preloaded about:newtab, and that happens to cause a problem for test_onLine.html.
The trick in test_onLine.html should resolve the issue. Let's try to land it.
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 2d08e02552b5 -d 9383659b5adc: rebasing 418322:2d08e02552b5 "Bug 1364364 - Part 1: Why are we declaring private virtual functions? r=smaug"
rebasing 418323:4eafaeb2c601 "Bug 1364364 - Part 2: Extract a part of EvictExpiredContentViewerForEntry to a new function FindTransactionForBFCache, and implement RemoveDynEntriesForEntry on top of it. r=smaug"
rebasing 418324:f5c6f4161363 "Bug 1364364 - Part 3: Clear dynamic subframe entries in RemoveFromBFCacheSync/Async. r=smaug"
rebasing 418325:d73a0ac7c5f7 "Bug 1364364 - Part 4: Add test case. r=smaug"
merging docshell/test/navigation/mochitest.ini
warning: conflicts while merging docshell/test/navigation/mochitest.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Manually rebased.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c9420f4e27f7
Part 1: Why are we declaring private virtual functions? r=smaug
https://hg.mozilla.org/integration/autoland/rev/5d6bf02b495a
Part 2: Extract a part of EvictExpiredContentViewerForEntry to a new function FindTransactionForBFCache, and implement RemoveDynEntriesForEntry on top of it. r=smaug
https://hg.mozilla.org/integration/autoland/rev/f23aa9861e9b
Part 3: Clear dynamic subframe entries in RemoveFromBFCacheSync/Async. r=smaug
https://hg.mozilla.org/integration/autoland/rev/6a0ec9eff8c3
Part 4: Add test case. r=smaug
https://hg.mozilla.org/integration/autoland/rev/6abd3298188a
Part 5.1: Rename frame/iframe.html of to-be-modifed test cases. r=smaug
https://hg.mozilla.org/integration/autoland/rev/285633e277cb
Part 5.2: Remove browser.sessionhistory.cache_subframes and fix test cases relying on it. r=smaug
https://hg.mozilla.org/integration/autoland/rev/6298563c1a81
Part 6: Re-connect nsDocViewerFocusListener in nsDocumentViewer::Open. r=smaug
Keywords: checkin-needed
I forgot the data: protocol is now cross-origin (and happen to be enabled after rebase). Updated the test cases.
https://reviewboard.mozilla.org/r/172544/diff/6-7/
Flags: needinfo?(sawang)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/24edc3618b45
Part 1: Why are we declaring private virtual functions? r=smaug
https://hg.mozilla.org/integration/autoland/rev/58fe37290a82
Part 2: Extract a part of EvictExpiredContentViewerForEntry to a new function FindTransactionForBFCache, and implement RemoveDynEntriesForEntry on top of it. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e07ddefd4a8e
Part 3: Clear dynamic subframe entries in RemoveFromBFCacheSync/Async. r=smaug
https://hg.mozilla.org/integration/autoland/rev/6fc41dc87a44
Part 4: Add test case. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d89fe870c4a8
Part 5.1: Rename frame/iframe.html of to-be-modifed test cases. r=smaug
https://hg.mozilla.org/integration/autoland/rev/0458fccefb5c
Part 5.2: Remove browser.sessionhistory.cache_subframes and fix test cases relying on it. r=smaug
https://hg.mozilla.org/integration/autoland/rev/f562d0ea59ef
Part 6: Re-connect nsDocViewerFocusListener in nsDocumentViewer::Open. r=smaug
Keywords: checkin-needed
Backed out for failing mochitest dom/workers/test/test_multi_sharedWorker_lifetimes.html on Android:

https://hg.mozilla.org/integration/autoland/rev/f2f37cfe64da87f7ff5a5e73a94dc333d73ec0ee
https://hg.mozilla.org/integration/autoland/rev/34f2787cfb1fc91e122c8121652f3b2cc9b03d62
https://hg.mozilla.org/integration/autoland/rev/aeae21d3975a0ba919ed6ada588d77ea6cdfee3a
https://hg.mozilla.org/integration/autoland/rev/75be59841099867b98458ba19c105f3180bade6a
https://hg.mozilla.org/integration/autoland/rev/33e477ed67969249c78ec4c2523162daf2d2cb99
https://hg.mozilla.org/integration/autoland/rev/38c2e85644f9f913dd47ec7bd3cda2b5b79a8fe9
https://hg.mozilla.org/integration/autoland/rev/710a16034611d6d4860847a493c77b2cfb97914f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f562d0ea59ef5967b3dc74d1f836c04ed4b97286&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=129239506&repo=autoland

[task 2017-09-07T14:29:29.418998Z] 14:29:29     INFO -  556 INFO Navigating to multi_sharedWorker_frame.html
[task 2017-09-07T14:29:29.419268Z] 14:29:29     INFO -  Buffered messages logged at 14:29:23
[task 2017-09-07T14:29:29.419685Z] 14:29:29     INFO -  557 INFO Posting message: {"command":"retrieve"}
[task 2017-09-07T14:29:29.420099Z] 14:29:29     INFO -  558 INFO Worker message: {"type":"result"}
[task 2017-09-07T14:29:29.420505Z] 14:29:29     INFO -  559 INFO TEST-PASS | dom/workers/test/test_multi_sharedWorker_lifetimes.html | Got a MessageEvent
[task 2017-09-07T14:29:29.420964Z] 14:29:29     INFO -  560 INFO TEST-PASS | dom/workers/test/test_multi_sharedWorker_lifetimes.html | Correct window got the event
[task 2017-09-07T14:29:29.421365Z] 14:29:29     INFO -  561 INFO TEST-PASS | dom/workers/test/test_multi_sharedWorker_lifetimes.html | Got a result message
[task 2017-09-07T14:29:29.421759Z] 14:29:29     INFO -  Buffered messages finished
[task 2017-09-07T14:29:29.422245Z] 14:29:29     INFO -  562 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/test_multi_sharedWorker_lifetimes.html | Still have data stored - got undefined, expected "0123456789abcdefghijklmnopqrstuvwxyz"
[task 2017-09-07T14:29:29.422610Z] 14:29:29     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2017-09-07T14:29:29.423059Z] 14:29:29     INFO -      testGenerator<@dom/workers/test/test_multi_sharedWorker_lifetimes.html:124:11
[task 2017-09-07T14:29:29.423480Z] 14:29:29     INFO -      testGenerator</<@dom/workers/test/test_multi_sharedWorker_lifetimes.html:34:15
Flags: needinfo?(sawang)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/de6c153ec533
Part 1: Why are we declaring private virtual functions? r=smaug
https://hg.mozilla.org/integration/autoland/rev/074475da0f2c
Part 2: Extract a part of EvictExpiredContentViewerForEntry to a new function FindTransactionForBFCache, and implement RemoveDynEntriesForEntry on top of it. r=smaug
https://hg.mozilla.org/integration/autoland/rev/21ee8f318a47
Part 3: Clear dynamic subframe entries in RemoveFromBFCacheSync/Async. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c5a737bbfdeb
Part 4: Add test case. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c394b06dc30c
Part 5.1: Rename frame/iframe.html of to-be-modifed test cases. r=smaug
https://hg.mozilla.org/integration/autoland/rev/809036cfd7d9
Part 5.2: Remove browser.sessionhistory.cache_subframes and fix test cases relying on it. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c517d8071dfb
Part 6: Re-connect nsDocViewerFocusListener in nsDocumentViewer::Open. r=smaug
Keywords: checkin-needed
Depends on: 1399182
Backed out for causing bug 1399182.

https://hg.mozilla.org/mozilla-central/rev/4069ad6982eabbab5b30fd45de159fe746e5a517
Status: RESOLVED → REOPENED
status-firefox57: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla57 → ---
Updated the patch to address bug 1399182, but to minimize the risk I'm not going to land it on 57. Let's wait until the final beta merge of 57 on 9/20, and let this ride the 58 train.
m-c is now 58. Let's land it again.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/98be8259bebe
Part 1: Why are we declaring private virtual functions? r=smaug
https://hg.mozilla.org/integration/autoland/rev/0177f0fcd043
Part 2: Extract a part of EvictExpiredContentViewerForEntry to a new function FindTransactionForBFCache, and implement RemoveDynEntriesForEntry on top of it. r=smaug
https://hg.mozilla.org/integration/autoland/rev/1422e5ec0a70
Part 3: Clear dynamic subframe entries in RemoveFromBFCacheSync/Async. r=smaug
https://hg.mozilla.org/integration/autoland/rev/cbc5a2fd926e
Part 4: Add test case. r=smaug
https://hg.mozilla.org/integration/autoland/rev/0b1860065e9d
Part 5.1: Rename frame/iframe.html of to-be-modifed test cases. r=smaug
https://hg.mozilla.org/integration/autoland/rev/3ccea0da4b20
Part 5.2: Remove browser.sessionhistory.cache_subframes and fix test cases relying on it. r=smaug
https://hg.mozilla.org/integration/autoland/rev/5f3fdf98aadc
Part 6: Re-connect nsDocViewerFocusListener in nsDocumentViewer::Open. r=smaug
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: