Closed Bug 1315233 Opened 3 years ago Closed 3 years ago

Crash in mozilla::dom::PostMessageEvent::Run

Categories

(Core :: DOM: Events, defect, P1, critical)

Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox50 --- unaffected
firefox51 + fixed
firefox52 --- fixed

People

(Reporter: marcia, Assigned: kmag)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-eacd3e4e-db24-4f20-845f-811762161104.
=============================================================

Seen while looking at nightly crash stats: http://bit.ly/2fj02jL. Crash started with the 20161103030205 build.
Low volume. baku, wanna take a quick look?
Flags: needinfo?(amarchesini)
This is a regression from bug 1308920
Blocks: 1308920
Flags: needinfo?(amarchesini) → needinfo?(kmaglione+bmo)
[Tracking Requested - why for this release]:
bug 1308920 landed to FF50 too.
The crash is from a diagnostic assert, so 50 isn't affected.

It looks like this is an add-on bug, but I'm still looking into the details. The most common correlation is with Norton Security Toolbar, and it definitely does some strange things with postMessage, but I only have access to obfuscated sources, and I can't tell yet whether this is being caused by the calls in PageMod scripts or in its other ad-hoc usage.
Flags: needinfo?(kmaglione+bmo)
Assuming I've managed to correctly decipher this over-engineered mess, I'm pretty sure the problem with the Norton add-ons is caused by the SDK's frame module:

http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/ui/frame/model.js
http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/ui/frame/view.js

After a few dozen layers of indirection, the postMessage call from the add-on code to the frame object gets dispatched via a postMessage call to an <iframe> content window. Which normally works fine. Except in permanent private browsing mode, where the frame contents have a privateBrowsingId origin attribute, but the sandbox that eventually winds up calling postMessage does not.
This crash signature is the number 4 top crash for the 11-4 Nightly.
Keywords: regression, topcrash
Track 51+ as this is #6 top crash in 51 aurora.
Comment on attachment 8807918 [details]
Bug 1315233: Allow window.postMessage from system principal with mismatched origin attributes.

https://reviewboard.mozilla.org/r/90890/#review90732

Can you add a test?
Attachment #8807918 - Flags: review?(amarchesini) → review+
Assignee: nobody → kmaglione+bmo
Priority: -- → P1
(In reply to Andrea Marchesini [:baku] from comment #9)
> Can you add a test?

Yeah. I was initially intending to, but none of the related OA changes to this code had any tests to go with them.
Keywords: leave-open
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d74bafb4a41e
Allow window.postMessage from system principal with mismatched origin attributes. r=baku
Comment on attachment 8808322 [details]
Bug 1315233: Add test for postMessage from system principal to window with non-default originAttributes.

https://reviewboard.mozilla.org/r/91150/#review91154
Attachment #8808322 - Flags: review?(amarchesini) → review+
Comment on attachment 8807918 [details]
Bug 1315233: Allow window.postMessage from system principal with mismatched origin attributes.

Approval Request Comment
[Feature/regressing bug #]: Bug 1308920
[User impact if declined]: This fixes a topcrash in Aurora, and also fixes some add-on compatibility issues with private browsing windows and permanent private browsing mode that trigger the crash.
[Describe test coverage new/current, TreeHerder]: A follow-up patch adds tests for this issue.
[Risks and why]: Low. This change simply exempts callers with the system principal from origin attribute checks when calling window.postMessage. Unprivileged callers are unaffected.
[String/UUID change made/needed]: None.
Attachment #8807918 - Flags: approval-mozilla-aurora?
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/429f3d73d79b
Add test for postMessage from system principal to window with non-default originAttributes. r=baku
Comment on attachment 8807918 [details]
Bug 1315233: Allow window.postMessage from system principal with mismatched origin attributes.

Fix a top crash. Take it in 51 aurora.
Attachment #8807918 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
To be completely explicit - bug 1308920 never landed in 50?  I don't see the mention of it, but I see the approval, and bug 1308920 comment 56 is pointing in that direction.  If this is in 50, then we need to uplift all the way to beta.
Flags: needinfo?(kmaglione+bmo)
(In reply to Milan Sreckovic [:milan] from comment #19)
> To be completely explicit - bug 1308920 never landed in 50?  I don't see the
> mention of it, but I see the approval, and bug 1308920 comment 56 is
> pointing in that direction.  If this is in 50, then we need to uplift all
> the way to beta.

Bug 1308920 landed in 50, but the crash is caused by a diagnostic assert, which has no effect in non-debug beta builds.
Flags: needinfo?(kmaglione+bmo)
needs rebasing for aurora

grafting 373563:d74bafb4a41e "Bug 1315233: Allow window.postMessage from system principal with mismatched origin attributes. r=baku"
merging dom/base/nsGlobalWindow.cpp
warning: conflicts while merging dom/base/nsGlobalWindow.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(kmaglione+bmo)
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Reopening, since a few crashes have now shown up in builds that have the fix. Those also seem to be extension issues, but of a different nature, and much lower volume.
Status: RESOLVED → REOPENED
Keywords: topcrash
Resolution: FIXED → ---
It looks like the new crashes are caused by the Private Tab add-on: https://addons.mozilla.org/firefox/addon/private-tab/

It does things like opening channels and setting the usePrivateBrowsing flag on their load context to true, and then repeatedly doing so again at various intervals. Crashing is actually probably the correct behavior, in this case.

Jorge or Andreas, can you please add a compatibility override to this add-on for Firefox 51+? We should also consider just outright blocklisting it.
Flags: needinfo?(jorge)
Flags: needinfo?(awagner)
I don't have access to either the compat override page or the blocklist tool.
Flags: needinfo?(awagner)
Flags: needinfo?(awilliamson)
Has/can someone notify the developer to explain what should be changed?  The most recent update was only in August, mentioning 51, so it's not abandoned.
I'm not sure there's any fixing it, and if there is I can't explain how. The things its doing with protocol handlers are not safe, and aren't meant to be supported. The rest of the add-on is a terrifying mess of eval-based monkey patching that I really don't want to wade into.

But the short of it is: The add-on is causing content windows with private browsing origin attributes to have access to other content windows with different private browsing origin attributes, but the same origin otherwise. Those windows are trying to communicate with each other, and causing a crash. That's what needs to be fixed: content windows with different origin attributes should never be able to get references to each other.
I added the compat override and directed the developer to #c27
Flags: needinfo?(awilliamson)
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
(In reply to Kris Maglione [:kmag] from comment #27)
> The things its doing with protocol handlers are not safe, and aren't meant to be
> supported.

So, custom nsIProtocolHandler implementation, that will change nsILoadContext.usePrivateBrowsing "aren't supported"?
Or how implement such feature in safe way?

> The rest of the add-on is a terrifying mess of eval-based monkey
> patching that I really don't want to wade into.

Really?
The main part is just about .usePrivateBrowsing = true right after "TabOpen" event.
Also there is only wrappers like
var orig = fn;
fn = function() {
    // do something
    return orig.aply(this, arguments);
};

> But the short of it is: The add-on is causing content windows with private
> browsing origin attributes to have access to other content windows with
> different private browsing origin attributes, but the same origin otherwise.
> Those windows are trying to communicate with each other, and causing a
> crash. That's what needs to be fixed: content windows with different origin
> attributes should never be able to get references to each other.

OK. How I can modify "the same origin" things or do something else from Private Tab side to forbid such access? Any API?
(In reply to Kris Maglione [:kmag] from comment #24)
> It does things like opening channels and setting the usePrivateBrowsing flag
> on their load context to true, and then repeatedly doing so again at various
> intervals. Crashing is actually probably the correct behavior, in this case.

Where is "repeatedly doing so again at various intervals"?
https://github.com/Infocatcher/Private_Tab/blob/0.2.0/protocol.jsm#L94-L98
Or this is about something else? Please explain.


Also... The most important question.
How correctly implement private tabs?
Flags: needinfo?(jorge)
This bug report is about the crashes, which were resolved. Please move the technical discussion to the forum or one of our other developer channels: https://developer.mozilla.org/en-US/Add-ons#Contact_us
Blocks: 1318388
(In reply to Jorge Villalobos [:jorgev] from comment #31)
> This bug report is about the crashes, which were resolved. Please move the
> technical discussion to the forum or one of our other developer channels:
> https://developer.mozilla.org/en-US/Add-ons#Contact_us

OK, I've created separated bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1318388
(In reply to Infocatcher from comment #29)
> (In reply to Kris Maglione [:kmag] from comment #27)
> > The things its doing with protocol handlers are not safe, and aren't meant to be
> > supported.
> 
> So, custom nsIProtocolHandler implementation, that will change
> nsILoadContext.usePrivateBrowsing "aren't supported"?

Correct. That information should come from the load context of the docShell,
which should match the origin attributes of the principals loaded into the
docShell, and should never change after the docShell is created.

You're currently (repeatedly) changing the private browsing status of a
docShell when it loads a certain channel, which is very much not supported.

> > The rest of the add-on is a terrifying mess of eval-based monkey
> > patching that I really don't want to wade into.
>
> Really?

Yes, but let's not get into it. If it were up to me, any amount of eval-based
monkey patching would have been banned a long time ago. And when it's tied up
with intricate, security-sensitive code, it makes things especially difficult
to reason about. But, in any case, that comment was meant for Andrew, who
should know the context that I intended it to be taken in.

> OK. How I can modify "the same origin" things or do something else from
> Private Tab side to forbid such access? Any API?

The only plausibly safe way to do this is to set the private browsing ID on
the origin attributes of the tab's top-level docShell when it's created, but
"plausibly safe" is as far as I think that will take you.

(In reply to Infocatcher from comment #30)
> (In reply to Kris Maglione [:kmag] from comment #24)
> > It does things like opening channels and setting the usePrivateBrowsing flag
> > on their load context to true, and then repeatedly doing so again at various
> > intervals. Crashing is actually probably the correct behavior, in this case.
> 
> Where is "repeatedly doing so again at various intervals"?
> https://github.com/Infocatcher/Private_Tab/blob/0.2.0/protocol.jsm#L94-L98

Yes.

> Also... The most important question.
> How correctly implement private tabs?

See above, but I'm not sure there's really any "correct" way without a lot of
platform changes, at this point. But we can continue this conversation in the
other bug.
As an user of the PrivateTab addon I hope Firefox developers will actively help to make this addon compatible. It should be a native feature but at least you should make thinks easy to people developing it.
You need to log in before you can comment on or make changes to this bug.