servo: Merge #15906 - Access browsing context safely (from mchv:mchv-safe-browsing-context); r=asajeffrey
authorMariot Chauvin <mariot.chauvin@gmail.com>
Thu, 16 Mar 2017 15:41:07 -0700
changeset 348138 2f61dd1624543ba8d464ec2e1a39f6064cd37e21
parent 348137 0efb2fdb3ee884ac6a3241a09fd3f1536edee41e
child 348139 ce8f1cde004fe99c5a5c4619549314b174f4dd25
push id88164
push usercbook@mozilla.com
push dateFri, 17 Mar 2017 13:55:35 +0000
treeherdermozilla-inbound@e46c08babe02 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasajeffrey
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #15906 - Access browsing context safely (from mchv:mchv-safe-browsing-context); r=asajeffrey Current browsing context accessor in `Document` unwraps the browsing context `Option` which prevents document to handle correctly the case when there is no browsing context. This is the reason servo panics with `session-history.max-length=1` (https://github.com/servo/servo/issues/15877). As it is my first contribution, I added a `safe` method to retrieve the browsing context rather than change the existing method, but I am happy to change if you think this is the right approach. I did not as well replace all existing method call to the `safe` method, to focus on fixing the issue. If someone can give me a bit of guidance for the test, I will try to contribute one. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [X] These changes fix #15877. - [ ] There are tests for these changes <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: e62d029ed643e5ce503cfbc7525da75a705ccf71
servo/components/script/dom/document.rs
servo/components/script/dom/window.rs
--- a/servo/components/script/dom/document.rs
+++ b/servo/components/script/dom/document.rs
@@ -393,17 +393,17 @@ impl Document {
 
     #[inline]
     pub fn has_browsing_context(&self) -> bool { self.has_browsing_context }
 
     /// https://html.spec.whatwg.org/multipage/#concept-document-bc
     #[inline]
     pub fn browsing_context(&self) -> Option<Root<BrowsingContext>> {
         if self.has_browsing_context {
-            Some(self.window.browsing_context())
+            self.window.maybe_browsing_context()
         } else {
             None
         }
     }
 
     #[inline]
     pub fn window(&self) -> &Window {
         &*self.window
--- a/servo/components/script/dom/window.rs
+++ b/servo/components/script/dom/window.rs
@@ -314,20 +314,25 @@ impl Window {
         let (tx, rx) = channel();
         (box SendableMainThreadScriptChan(tx), box rx)
     }
 
     pub fn image_cache_thread(&self) -> &ImageCacheThread {
         &self.image_cache_thread
     }
 
+    /// This can panic if it is called after the browsing context has been discarded
     pub fn browsing_context(&self) -> Root<BrowsingContext> {
         self.browsing_context.get().unwrap()
     }
 
+    pub fn maybe_browsing_context(&self) -> Option<Root<BrowsingContext>> {
+        self.browsing_context.get()
+    }
+
     pub fn bluetooth_thread(&self) -> IpcSender<BluetoothRequest> {
         self.bluetooth_thread.clone()
     }
 
     pub fn bluetooth_extra_permission_data(&self) -> &BluetoothExtraPermissionData {
          &self.bluetooth_extra_permission_data
     }
 
@@ -622,31 +627,31 @@ impl WindowMethods for Window {
     // https://html.spec.whatwg.org/multipage/#dom-frames
     fn Frames(&self) -> Root<BrowsingContext> {
         self.browsing_context()
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-parent
     fn GetParent(&self) -> Option<Root<BrowsingContext>> {
         // Steps 1. and 2.
-        if self.browsing_context().is_discarded() {
+        if self.maybe_browsing_context().map_or(true, |c| c.is_discarded()) {
             return None;
         }
         match self.parent() {
             // Step 4.
             Some(parent) => Some(parent.Window()),
             // Step 5.
             None => Some(self.Window())
         }
      }
 
     // https://html.spec.whatwg.org/multipage/#dom-top
     fn GetTop(&self) -> Option<Root<BrowsingContext>> {
         // Steps 1. and 2.
-        if self.browsing_context().is_discarded() {
+        if self.maybe_browsing_context().map_or(true, |c| c.is_discarded()) {
             return None;
         }
         // Step 5.
         let mut window = Root::from_ref(self);
         while let Some(parent) = window.parent() {
             window = parent;
         }
         Some(window.Window())