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)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: freesamael, Assigned: freesamael)
References
Details
Attachments
(7 files)
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
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
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → sawang
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
WIP patches. Still thinking how we can test this.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
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
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
mozreview-review |
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 12•7 years ago
|
||
mozreview-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 13•7 years ago
|
||
mozreview-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 14•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 15•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c031cf1919ae429667e95f3786aec11b522d175b
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-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 20•7 years ago
|
||
mozreview-review |
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 21•7 years ago
|
||
mozreview-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 22•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 23•7 years ago
|
||
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
Assignee | ||
Comment 24•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•7 years ago
|
||
The trick in test_onLine.html should resolve the issue. Let's try to land it.
Keywords: checkin-needed
Comment 30•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 38•7 years ago
|
||
Manually rebased.
Comment 39•7 years ago
|
||
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
Backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=129015286&repo=autoland https://hg.mozilla.org/integration/autoland/rev/f9c03c26c876
Flags: needinfo?(sawang)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 43•7 years ago
|
||
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
Comment 44•7 years ago
|
||
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
Comment 45•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 48•7 years ago
|
||
try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9447f4cf9670a59fd5437dd93b3566dd5bf591c4
Flags: needinfo?(sawang)
Keywords: checkin-needed
Comment 49•7 years ago
|
||
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
Comment 50•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/de6c153ec533 https://hg.mozilla.org/mozilla-central/rev/074475da0f2c https://hg.mozilla.org/mozilla-central/rev/21ee8f318a47 https://hg.mozilla.org/mozilla-central/rev/c5a737bbfdeb https://hg.mozilla.org/mozilla-central/rev/c394b06dc30c https://hg.mozilla.org/mozilla-central/rev/809036cfd7d9 https://hg.mozilla.org/mozilla-central/rev/c517d8071dfb
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 51•7 years ago
|
||
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 → ---
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 57•7 years ago
|
||
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.
Comment 59•7 years ago
|
||
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
Comment 60•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/98be8259bebe https://hg.mozilla.org/mozilla-central/rev/0177f0fcd043 https://hg.mozilla.org/mozilla-central/rev/1422e5ec0a70 https://hg.mozilla.org/mozilla-central/rev/cbc5a2fd926e https://hg.mozilla.org/mozilla-central/rev/0b1860065e9d https://hg.mozilla.org/mozilla-central/rev/3ccea0da4b20 https://hg.mozilla.org/mozilla-central/rev/5f3fdf98aadc
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•