Crash when removing srcdoc attribute from iframe

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jkitch, Assigned: jkitch)

Tracking

({crash})

unspecified
mozilla25
x86
Windows 8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

6 years ago
Posted file manual testcase
There appears to be a crash related to removing the srcdoc attribute from an iframe.

Steps to reproduce:

1. Compile/run a debug build of Nightly.
2. Load the attached testcase
3. Reload the page

Result
Null-deref crash in content/base/src/nsContentUtils:6111

>    if (aForceOwner) {
>      nsAutoCString uriStr;
>      aURI->GetSpec(uriStr);
>      if(!uriStr.EqualsLiteral("about:srcdoc")) {
>        nsCOMPtr<nsIURI> ownerURI;
>        nsresult rv = aLoadingPrincipal->GetURI(getter_AddRefs(ownerURI)); //offending line
>        MOZ_ASSERT(NS_SUCCEEDED(rv) && SchemeIs(ownerURI, NS_NULLPRINCIPAL_SCHEME));
>      }
>    }
Assignee

Updated

6 years ago
Assignee: nobody → jkitch.bug

Updated

6 years ago
Keywords: crash
Fwiw, I can't reproduce so far...
Assignee

Comment 2

6 years ago
Posted patch fix + test (obsolete) — Splinter Review
I've identified the cause.  nsSHEntry instances are reused when the nsIWebNavigation::LOAD_FLAGS_REPLACE_HISTORY flag is set.  Before this patch, mIsSrcdocEntry wasn't touched by the Create() function, so the previous srcdoc status is inherited by the new entry, causing it to be wrongly flagged as a srcdoc entry.

Because the URI is correctly updated to the new URI, the exception to deal with srcdoc entries in nsContentUtils::SetUpChannelOwner() isn't triggered, resulting in the crash.

The attached patch resets mIsSrcdocEntry whenever the nsSHEntry instance is reused.

Regarding the original testcase: The testcase needs to be run from a local disk as https/the mixed content blocker appears to be affecting the outcome.   The browser must also be a debug build.  (It may also need Windows, I haven't tried other platforms).
Attachment #776312 - Flags: review?(bzbarsky)
Assignee

Comment 3

6 years ago
Posted patch fix + testSplinter Review
Removed some commented out, legacy test code.
Attachment #776312 - Attachment is obsolete: true
Attachment #776312 - Flags: review?(bzbarsky)
Attachment #776318 - Flags: review?(bzbarsky)
Comment on attachment 776318 [details] [diff] [review]
fix + test

r=me
Attachment #776318 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/964be82efbf3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.